Rewrite the uSockets core in Rust - #34037
Conversation
…umer migration in progress
…er-target outputs
… MADV reclamation, unified polls, loop-shared TLS spill
… from consumers, FilePolls unified
Replaces the C socket core with a Rust crate: generational slab-backed sockets (stale handles are safe no-ops), all unsafe jailed in one audited module, loop-shared TLS staging over build-time bssl-sys bindings, const link-time dispatch tables, and a unified poll registry for sockets and file polls. Consumers migrate to a safe callback interface where the core holds the owner alive across dispatch. Deletes ~21k lines of C and Rust layout mirrors; semantics are parity-ported and documented in src/usockets/docs/.
Ports the deleted C Windows branches: nonblocking connect via FIONBIO, WSAEWOULDBLOCK mapping to the shared would-block convention, CRT errno translation, SetHandleInformation no-inherit, SIO_UDP_CONNRESET/NETRESET off for UDP, closesocket teardown.
|
Updated 2:42 PM PT - Jul 13th, 2026
❌ @Jarred-Sumner, your commit b061f12 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34037That installs a local version of the PR into your bun-34037 --bun |
… build-time bindgen CI builders don't carry bindgen/libclang. Bindings for all 12 targets are committed and selected by build.rs per TARGET; regenerate.sh refreshes them on BoringSSL bumps, with the pinned-commit stamp verified at both configure and cargo time so a stale copy fails loudly.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR replaces the C/C++ uSockets implementation (packages/bun-usockets, packages/bun-uws) with a new Rust crate ChangesuSockets Rust rewrite
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/event_loop/SpawnSyncEventLoop.rs`:
- Around line 379-389: Reuse the existing uv_loop local from the timer
initialization block when calling libuv::uv_update_time, instead of recomputing
self.uws_loop().uv_loop.cast(). Keep the existing safety context and timer
behavior unchanged.
In `@src/usockets/tls/context.rs`:
- Around line 143-175: The X509 error-code mapping is duplicated between
x509_error_code and the state.rs variant. Make the state.rs helper delegate to
x509_error_code and convert its returned CStr with as_ptr(), preserving the
existing state.rs return type and leaving a single mapping source of truth.
In `@src/uws_shim/App.rs`:
- Around line 143-163: Deduplicate the publish entry points: update either
publish_with_options or publish to delegate to the other instead of
independently calling c::uws_publish, while preserving the existing arguments
and SendStatus behavior. Apply the same consolidation to the corresponding
implementation around the second referenced method, and remove the now-redundant
byte-identical logic.
In `@src/uws_shim/BodyReaderMixin.rs`:
- Around line 156-207: Extract the duplicated body accumulation logic from
on_data into a shared helper, such as append_chunk, that performs the
MAX_BODY_SIZE check, try_reserve, and extend_from_slice operations and returns
the existing errors. Replace both the non-last branch and the last branch’s
non-empty accumulator sequence with calls to this helper, preserving their
current ownership and response-handling behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 547e73f9-2269-4244-9d1c-2bbbbdab8b56
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (252)
.gitignoreCargo.tomlpackages/bun-usockets/src/bsd.cpackages/bun-usockets/src/context.cpackages/bun-usockets/src/crypto/openssl.cpackages/bun-usockets/src/crypto/root_certs.cpppackages/bun-usockets/src/crypto/sni_tree.cpppackages/bun-usockets/src/eventing/epoll_kqueue.cpackages/bun-usockets/src/eventing/libuv.cpackages/bun-usockets/src/fault_inject.cpackages/bun-usockets/src/internal/eventing/epoll_kqueue.hpackages/bun-usockets/src/internal/eventing/libuv.hpackages/bun-usockets/src/internal/fault_inject.hpackages/bun-usockets/src/internal/internal.hpackages/bun-usockets/src/internal/loop_data.hpackages/bun-usockets/src/internal/networking/bsd.hpackages/bun-usockets/src/libusockets_cabi.hpackages/bun-usockets/src/loop.cpackages/bun-usockets/src/quic.cpackages/bun-usockets/src/socket.cpackages/bun-usockets/src/udp.cpackages/bun-uws/src/App.hpackages/bun-uws/src/AsyncSocket.hpackages/bun-uws/src/HttpContext.hpackages/bun-uws/src/HttpResponse.hpatches/boringssl/bssl-sys-prebuilt-bindings.patchscripts/build/bun.tsscripts/build/deps/boringssl.tsscripts/build/rust.tsscripts/build/source.tsscripts/glob-sources.tssrc/bun_bin/Cargo.tomlsrc/bun_bin/lib.rssrc/dns/lib.rssrc/event_loop/AnyEventLoop.rssrc/event_loop/Cargo.tomlsrc/event_loop/MiniEventLoop.rssrc/event_loop/README.mdsrc/event_loop/SpawnSyncEventLoop.rssrc/event_loop/lib.rssrc/http/Cargo.tomlsrc/http/CertificateInfo.rssrc/http/HTTPCertError.rssrc/http/HTTPContext.rssrc/http/HTTPThread.rssrc/http/ProxyTunnel.rssrc/http/h2_client/ClientSession.rssrc/http/h3_client/ClientContext.rssrc/http/h3_client/ClientSession.rssrc/http/h3_client/PendingConnect.rssrc/http/h3_client/Stream.rssrc/http/h3_client/callbacks.rssrc/http/h3_client/encode.rssrc/http/lib.rssrc/http/ssl_config.rssrc/http_jsc/Cargo.tomlsrc/http_jsc/websocket_client.rssrc/http_jsc/websocket_client/CppWebSocket.rssrc/http_jsc/websocket_client/WebSocketProxyTunnel.rssrc/http_jsc/websocket_client/WebSocketUpgradeClient.rssrc/install/Cargo.tomlsrc/install/PackageManager/security_scanner.rssrc/install/lifecycle_script_runner.rssrc/install/resolution.rssrc/io/Cargo.tomlsrc/io/PipeReader.rssrc/io/PipeWriter.rssrc/io/lib.rssrc/io/posix_event_loop.rssrc/io/windows_event_loop.rssrc/jsc/Cargo.tomlsrc/jsc/Debugger.rssrc/jsc/FetchHeaders.rssrc/jsc/GarbageCollectionController.rssrc/jsc/SystemError.rssrc/jsc/VirtualMachine.rssrc/jsc/bindings/bindings.cppsrc/jsc/event_loop.rssrc/jsc/ipc.rssrc/jsc/lib.rssrc/jsc/rare_data.rssrc/jsc/virtual_machine_exports.rssrc/jsc/web_worker.rssrc/react_compiler/lowering/build_hir/mod.rssrc/resolver/lib.rssrc/router/lib.rssrc/runtime/Cargo.tomlsrc/runtime/api/bun/SSLContextCache.rssrc/runtime/api/bun/SecureContext.rssrc/runtime/api/bun/Terminal.rssrc/runtime/api/bun/h2_frame_parser.rssrc/runtime/api/bun/js_bun_spawn_bindings.rssrc/runtime/api/bun/subprocess.rssrc/runtime/api/bun/subprocess/SubprocessPipeReader.rssrc/runtime/bake/DevServer.rssrc/runtime/bake/dev_server/error_report_request.rssrc/runtime/bake/dev_server/hmr_socket.rssrc/runtime/bake/dev_server/mod.rssrc/runtime/cli/test/parallel/Channel.rssrc/runtime/cli/test/parallel/Coordinator.rssrc/runtime/cli/test/parallel/Worker.rssrc/runtime/cli/test/parallel/runner.rssrc/runtime/crypto/CryptoHasher.rssrc/runtime/dispatch.rssrc/runtime/dns_jsc/dns.rssrc/runtime/ffi/ffi_body.rssrc/runtime/hw_exports.rssrc/runtime/ipc_host.rssrc/runtime/jsc_hooks.rssrc/runtime/node/node_cluster_binding.rssrc/runtime/node/node_net_binding.rssrc/runtime/node/node_os.rssrc/runtime/server/AnyRequestContext.rssrc/runtime/server/FileResponseStream.rssrc/runtime/server/FileRoute.rssrc/runtime/server/HTMLBundle.rssrc/runtime/server/NodeHTTPResponse.rssrc/runtime/server/RangeRequest.rssrc/runtime/server/RequestContext.rssrc/runtime/server/ServerConfig.rssrc/runtime/server/ServerWebSocket.rssrc/runtime/server/StaticRoute.rssrc/runtime/server/WebSocketServerContext.rssrc/runtime/server/mod.rssrc/runtime/server/server_body.rssrc/runtime/shell/IOWriter.rssrc/runtime/shell/builtin/yes.rssrc/runtime/shell/shell_body.rssrc/runtime/socket/Handlers.rssrc/runtime/socket/Listener.rssrc/runtime/socket/UpgradedDuplex.rssrc/runtime/socket/WindowsNamedPipe.rssrc/runtime/socket/WindowsNamedPipeContext.rssrc/runtime/socket/mod.rssrc/runtime/socket/socket_body.rssrc/runtime/socket/tls_socket_functions.rssrc/runtime/socket/udp_socket.rssrc/runtime/socket/uws_dispatch.rssrc/runtime/socket/uws_handlers.rssrc/runtime/socket/uws_jsc.rssrc/runtime/test_runner/diff/diff_match_patch.rssrc/runtime/timer/DateHeaderTimer.rssrc/runtime/timer/Timer.rssrc/runtime/timer/WTFTimer.rssrc/runtime/timer/mod.rssrc/runtime/valkey_jsc/js_valkey.rssrc/runtime/valkey_jsc/mod.rssrc/runtime/valkey_jsc/valkey.rssrc/runtime/webcore/CookieMap.rssrc/runtime/webcore/FileReader.rssrc/runtime/webcore/Request.rssrc/runtime/webcore/fetch/FetchTasklet.rssrc/runtime/webcore/streams.rssrc/spawn/Cargo.tomlsrc/spawn/process.rssrc/sql_jsc/Cargo.tomlsrc/sql_jsc/jsc.rssrc/sql_jsc/mysql/JSMySQLConnection.rssrc/sql_jsc/mysql/MySQLConnection.rssrc/sql_jsc/postgres/PostgresSQLConnection.rssrc/sql_jsc/shared/ConnectionCtorArgs.rssrc/usockets/Cargo.tomlsrc/usockets/backend/epoll.rssrc/usockets/backend/kqueue.rssrc/usockets/backend/libuv.rssrc/usockets/backend/mod.rssrc/usockets/cabi.rssrc/usockets/connecting.rssrc/usockets/dispatch.rssrc/usockets/docs/cabi.mdsrc/usockets/docs/design.mdsrc/usockets/docs/semantics.mdsrc/usockets/docs/tls.mdsrc/usockets/fault.rssrc/usockets/group.rssrc/usockets/handle.rssrc/usockets/kind.rssrc/usockets/lib.rssrc/usockets/loop_/mod.rssrc/usockets/loop_/poll_registry.rssrc/usockets/loop_/tick.rssrc/usockets/loop_/timeouts.rssrc/usockets/loop_/wakeup.rssrc/usockets/protocol.rssrc/usockets/socket.rssrc/usockets/tls/bssl_bindings/README.mdsrc/usockets/tls/bssl_bindings/regenerate.shsrc/usockets/tls/bssl_bindings/wrapper.csrc/usockets/tls/bssl_bindings/wrapper_aarch64-apple-darwin.rssrc/usockets/tls/bssl_bindings/wrapper_aarch64-linux-android.rssrc/usockets/tls/bssl_bindings/wrapper_aarch64-pc-windows-msvc.rssrc/usockets/tls/bssl_bindings/wrapper_aarch64-unknown-freebsd.rssrc/usockets/tls/bssl_bindings/wrapper_aarch64-unknown-linux-gnu.rssrc/usockets/tls/bssl_bindings/wrapper_aarch64-unknown-linux-musl.rssrc/usockets/tls/bssl_bindings/wrapper_x86_64-apple-darwin.rssrc/usockets/tls/bssl_bindings/wrapper_x86_64-linux-android.rssrc/usockets/tls/bssl_bindings/wrapper_x86_64-pc-windows-msvc.rssrc/usockets/tls/bssl_bindings/wrapper_x86_64-unknown-freebsd.rssrc/usockets/tls/bssl_bindings/wrapper_x86_64-unknown-linux-gnu.rssrc/usockets/tls/bssl_bindings/wrapper_x86_64-unknown-linux-musl.rssrc/usockets/tls/context.rssrc/usockets/tls/mod.rssrc/usockets/tls/sni.rssrc/usockets/tls/state.rssrc/usockets/udp.rssrc/usockets/unsafe_core/bssl.rssrc/usockets/unsafe_core/deref.rssrc/usockets/unsafe_core/ext.rssrc/usockets/unsafe_core/ffi.rssrc/usockets/unsafe_core/io.rssrc/usockets/unsafe_core/mod.rssrc/usockets/unsafe_core/poll_access.rssrc/usockets/unsafe_core/slab.rssrc/usockets/unsafe_core/test_support.rssrc/usockets/unsafe_core/trampolines.rssrc/usockets/write.rssrc/uws_shim/App.rssrc/uws_shim/BodyReaderMixin.rssrc/uws_shim/Cargo.tomlsrc/uws_shim/Request.rssrc/uws_shim/Response.rssrc/uws_shim/WebSocket.rssrc/uws_shim/h3.rssrc/uws_shim/lib.rssrc/uws_shim/quic.rssrc/uws_shim/quic/Context.rssrc/uws_shim/quic/Header.rssrc/uws_shim/quic/PendingConnect.rssrc/uws_shim/quic/Socket.rssrc/uws_shim/quic/Stream.rssrc/uws_shim/thunk.rssrc/uws_shim/us_socket.rssrc/uws_sys/Cargo.tomlsrc/uws_sys/ConnectingSocket.rssrc/uws_sys/InternalLoopData.rssrc/uws_sys/ListenSocket.rssrc/uws_sys/Loop.rssrc/uws_sys/SocketContext.rssrc/uws_sys/SocketGroup.rssrc/uws_sys/SocketKind.rssrc/uws_sys/Timer.rssrc/uws_sys/lib.rssrc/uws_sys/libuwsockets.cppsrc/uws_sys/socket.rssrc/uws_sys/udp.rssrc/uws_sys/us_socket_t.rssrc/uws_sys/vtable.rstest/js/bun/net/socket-syscall-fault.test.tstest/js/bun/net/socket.test.tstest/js/node/http/node-http-syscall-fault.test.tstest/js/node/net/connect-autoselectfamily-cancel-loop-fixture.jstest/js/node/net/connect-autoselectfamily-stale-timer.test.ts
💤 Files with no reviewable changes (19)
- src/install/Cargo.toml
- packages/bun-usockets/src/internal/eventing/libuv.h
- packages/bun-uws/src/AsyncSocket.h
- packages/bun-usockets/src/internal/eventing/epoll_kqueue.h
- packages/bun-usockets/src/context.c
- packages/bun-usockets/src/internal/loop_data.h
- packages/bun-usockets/src/internal/fault_inject.h
- packages/bun-usockets/src/fault_inject.c
- src/jsc/bindings/bindings.cpp
- packages/bun-usockets/src/crypto/sni_tree.cpp
- packages/bun-usockets/src/internal/networking/bsd.h
- packages/bun-usockets/src/eventing/libuv.c
- packages/bun-usockets/src/internal/internal.h
- packages/bun-usockets/src/loop.c
- packages/bun-usockets/src/eventing/epoll_kqueue.c
- packages/bun-usockets/src/udp.c
- packages/bun-usockets/src/bsd.c
- packages/bun-usockets/src/socket.c
- src/uws_sys/libuwsockets.cpp
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/event_loop/SpawnSyncEventLoop.rs`:
- Around line 379-389: Reuse the existing uv_loop local from the timer
initialization block when calling libuv::uv_update_time, instead of recomputing
self.uws_loop().uv_loop.cast(). Keep the existing safety context and timer
behavior unchanged.
In `@src/usockets/tls/context.rs`:
- Around line 143-175: The X509 error-code mapping is duplicated between
x509_error_code and the state.rs variant. Make the state.rs helper delegate to
x509_error_code and convert its returned CStr with as_ptr(), preserving the
existing state.rs return type and leaving a single mapping source of truth.
In `@src/uws_shim/App.rs`:
- Around line 143-163: Deduplicate the publish entry points: update either
publish_with_options or publish to delegate to the other instead of
independently calling c::uws_publish, while preserving the existing arguments
and SendStatus behavior. Apply the same consolidation to the corresponding
implementation around the second referenced method, and remove the now-redundant
byte-identical logic.
In `@src/uws_shim/BodyReaderMixin.rs`:
- Around line 156-207: Extract the duplicated body accumulation logic from
on_data into a shared helper, such as append_chunk, that performs the
MAX_BODY_SIZE check, try_reserve, and extend_from_slice operations and returns
the existing errors. Replace both the non-last branch and the last branch’s
non-empty accumulator sequence with calls to this helper, preserving their
current ownership and response-handling behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 547e73f9-2269-4244-9d1c-2bbbbdab8b56
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (252)
.gitignoreCargo.tomlpackages/bun-usockets/src/bsd.cpackages/bun-usockets/src/context.cpackages/bun-usockets/src/crypto/openssl.cpackages/bun-usockets/src/crypto/root_certs.cpppackages/bun-usockets/src/crypto/sni_tree.cpppackages/bun-usockets/src/eventing/epoll_kqueue.cpackages/bun-usockets/src/eventing/libuv.cpackages/bun-usockets/src/fault_inject.cpackages/bun-usockets/src/internal/eventing/epoll_kqueue.hpackages/bun-usockets/src/internal/eventing/libuv.hpackages/bun-usockets/src/internal/fault_inject.hpackages/bun-usockets/src/internal/internal.hpackages/bun-usockets/src/internal/loop_data.hpackages/bun-usockets/src/internal/networking/bsd.hpackages/bun-usockets/src/libusockets_cabi.hpackages/bun-usockets/src/loop.cpackages/bun-usockets/src/quic.cpackages/bun-usockets/src/socket.cpackages/bun-usockets/src/udp.cpackages/bun-uws/src/App.hpackages/bun-uws/src/AsyncSocket.hpackages/bun-uws/src/HttpContext.hpackages/bun-uws/src/HttpResponse.hpatches/boringssl/bssl-sys-prebuilt-bindings.patchscripts/build/bun.tsscripts/build/deps/boringssl.tsscripts/build/rust.tsscripts/build/source.tsscripts/glob-sources.tssrc/bun_bin/Cargo.tomlsrc/bun_bin/lib.rssrc/dns/lib.rssrc/event_loop/AnyEventLoop.rssrc/event_loop/Cargo.tomlsrc/event_loop/MiniEventLoop.rssrc/event_loop/README.mdsrc/event_loop/SpawnSyncEventLoop.rssrc/event_loop/lib.rssrc/http/Cargo.tomlsrc/http/CertificateInfo.rssrc/http/HTTPCertError.rssrc/http/HTTPContext.rssrc/http/HTTPThread.rssrc/http/ProxyTunnel.rssrc/http/h2_client/ClientSession.rssrc/http/h3_client/ClientContext.rssrc/http/h3_client/ClientSession.rssrc/http/h3_client/PendingConnect.rssrc/http/h3_client/Stream.rssrc/http/h3_client/callbacks.rssrc/http/h3_client/encode.rssrc/http/lib.rssrc/http/ssl_config.rssrc/http_jsc/Cargo.tomlsrc/http_jsc/websocket_client.rssrc/http_jsc/websocket_client/CppWebSocket.rssrc/http_jsc/websocket_client/WebSocketProxyTunnel.rssrc/http_jsc/websocket_client/WebSocketUpgradeClient.rssrc/install/Cargo.tomlsrc/install/PackageManager/security_scanner.rssrc/install/lifecycle_script_runner.rssrc/install/resolution.rssrc/io/Cargo.tomlsrc/io/PipeReader.rssrc/io/PipeWriter.rssrc/io/lib.rssrc/io/posix_event_loop.rssrc/io/windows_event_loop.rssrc/jsc/Cargo.tomlsrc/jsc/Debugger.rssrc/jsc/FetchHeaders.rssrc/jsc/GarbageCollectionController.rssrc/jsc/SystemError.rssrc/jsc/VirtualMachine.rssrc/jsc/bindings/bindings.cppsrc/jsc/event_loop.rssrc/jsc/ipc.rssrc/jsc/lib.rssrc/jsc/rare_data.rssrc/jsc/virtual_machine_exports.rssrc/jsc/web_worker.rssrc/react_compiler/lowering/build_hir/mod.rssrc/resolver/lib.rssrc/router/lib.rssrc/runtime/Cargo.tomlsrc/runtime/api/bun/SSLContextCache.rssrc/runtime/api/bun/SecureContext.rssrc/runtime/api/bun/Terminal.rssrc/runtime/api/bun/h2_frame_parser.rssrc/runtime/api/bun/js_bun_spawn_bindings.rssrc/runtime/api/bun/subprocess.rssrc/runtime/api/bun/subprocess/SubprocessPipeReader.rssrc/runtime/bake/DevServer.rssrc/runtime/bake/dev_server/error_report_request.rssrc/runtime/bake/dev_server/hmr_socket.rssrc/runtime/bake/dev_server/mod.rssrc/runtime/cli/test/parallel/Channel.rssrc/runtime/cli/test/parallel/Coordinator.rssrc/runtime/cli/test/parallel/Worker.rssrc/runtime/cli/test/parallel/runner.rssrc/runtime/crypto/CryptoHasher.rssrc/runtime/dispatch.rssrc/runtime/dns_jsc/dns.rssrc/runtime/ffi/ffi_body.rssrc/runtime/hw_exports.rssrc/runtime/ipc_host.rssrc/runtime/jsc_hooks.rssrc/runtime/node/node_cluster_binding.rssrc/runtime/node/node_net_binding.rssrc/runtime/node/node_os.rssrc/runtime/server/AnyRequestContext.rssrc/runtime/server/FileResponseStream.rssrc/runtime/server/FileRoute.rssrc/runtime/server/HTMLBundle.rssrc/runtime/server/NodeHTTPResponse.rssrc/runtime/server/RangeRequest.rssrc/runtime/server/RequestContext.rssrc/runtime/server/ServerConfig.rssrc/runtime/server/ServerWebSocket.rssrc/runtime/server/StaticRoute.rssrc/runtime/server/WebSocketServerContext.rssrc/runtime/server/mod.rssrc/runtime/server/server_body.rssrc/runtime/shell/IOWriter.rssrc/runtime/shell/builtin/yes.rssrc/runtime/shell/shell_body.rssrc/runtime/socket/Handlers.rssrc/runtime/socket/Listener.rssrc/runtime/socket/UpgradedDuplex.rssrc/runtime/socket/WindowsNamedPipe.rssrc/runtime/socket/WindowsNamedPipeContext.rssrc/runtime/socket/mod.rssrc/runtime/socket/socket_body.rssrc/runtime/socket/tls_socket_functions.rssrc/runtime/socket/udp_socket.rssrc/runtime/socket/uws_dispatch.rssrc/runtime/socket/uws_handlers.rssrc/runtime/socket/uws_jsc.rssrc/runtime/test_runner/diff/diff_match_patch.rssrc/runtime/timer/DateHeaderTimer.rssrc/runtime/timer/Timer.rssrc/runtime/timer/WTFTimer.rssrc/runtime/timer/mod.rssrc/runtime/valkey_jsc/js_valkey.rssrc/runtime/valkey_jsc/mod.rssrc/runtime/valkey_jsc/valkey.rssrc/runtime/webcore/CookieMap.rssrc/runtime/webcore/FileReader.rssrc/runtime/webcore/Request.rssrc/runtime/webcore/fetch/FetchTasklet.rssrc/runtime/webcore/streams.rssrc/spawn/Cargo.tomlsrc/spawn/process.rssrc/sql_jsc/Cargo.tomlsrc/sql_jsc/jsc.rssrc/sql_jsc/mysql/JSMySQLConnection.rssrc/sql_jsc/mysql/MySQLConnection.rssrc/sql_jsc/postgres/PostgresSQLConnection.rssrc/sql_jsc/shared/ConnectionCtorArgs.rssrc/usockets/Cargo.tomlsrc/usockets/backend/epoll.rssrc/usockets/backend/kqueue.rssrc/usockets/backend/libuv.rssrc/usockets/backend/mod.rssrc/usockets/cabi.rssrc/usockets/connecting.rssrc/usockets/dispatch.rssrc/usockets/docs/cabi.mdsrc/usockets/docs/design.mdsrc/usockets/docs/semantics.mdsrc/usockets/docs/tls.mdsrc/usockets/fault.rssrc/usockets/group.rssrc/usockets/handle.rssrc/usockets/kind.rssrc/usockets/lib.rssrc/usockets/loop_/mod.rssrc/usockets/loop_/poll_registry.rssrc/usockets/loop_/tick.rssrc/usockets/loop_/timeouts.rssrc/usockets/loop_/wakeup.rssrc/usockets/protocol.rssrc/usockets/socket.rssrc/usockets/tls/bssl_bindings/README.mdsrc/usockets/tls/bssl_bindings/regenerate.shsrc/usockets/tls/bssl_bindings/wrapper.csrc/usockets/tls/bssl_bindings/wrapper_aarch64-apple-darwin.rssrc/usockets/tls/bssl_bindings/wrapper_aarch64-linux-android.rssrc/usockets/tls/bssl_bindings/wrapper_aarch64-pc-windows-msvc.rssrc/usockets/tls/bssl_bindings/wrapper_aarch64-unknown-freebsd.rssrc/usockets/tls/bssl_bindings/wrapper_aarch64-unknown-linux-gnu.rssrc/usockets/tls/bssl_bindings/wrapper_aarch64-unknown-linux-musl.rssrc/usockets/tls/bssl_bindings/wrapper_x86_64-apple-darwin.rssrc/usockets/tls/bssl_bindings/wrapper_x86_64-linux-android.rssrc/usockets/tls/bssl_bindings/wrapper_x86_64-pc-windows-msvc.rssrc/usockets/tls/bssl_bindings/wrapper_x86_64-unknown-freebsd.rssrc/usockets/tls/bssl_bindings/wrapper_x86_64-unknown-linux-gnu.rssrc/usockets/tls/bssl_bindings/wrapper_x86_64-unknown-linux-musl.rssrc/usockets/tls/context.rssrc/usockets/tls/mod.rssrc/usockets/tls/sni.rssrc/usockets/tls/state.rssrc/usockets/udp.rssrc/usockets/unsafe_core/bssl.rssrc/usockets/unsafe_core/deref.rssrc/usockets/unsafe_core/ext.rssrc/usockets/unsafe_core/ffi.rssrc/usockets/unsafe_core/io.rssrc/usockets/unsafe_core/mod.rssrc/usockets/unsafe_core/poll_access.rssrc/usockets/unsafe_core/slab.rssrc/usockets/unsafe_core/test_support.rssrc/usockets/unsafe_core/trampolines.rssrc/usockets/write.rssrc/uws_shim/App.rssrc/uws_shim/BodyReaderMixin.rssrc/uws_shim/Cargo.tomlsrc/uws_shim/Request.rssrc/uws_shim/Response.rssrc/uws_shim/WebSocket.rssrc/uws_shim/h3.rssrc/uws_shim/lib.rssrc/uws_shim/quic.rssrc/uws_shim/quic/Context.rssrc/uws_shim/quic/Header.rssrc/uws_shim/quic/PendingConnect.rssrc/uws_shim/quic/Socket.rssrc/uws_shim/quic/Stream.rssrc/uws_shim/thunk.rssrc/uws_shim/us_socket.rssrc/uws_sys/Cargo.tomlsrc/uws_sys/ConnectingSocket.rssrc/uws_sys/InternalLoopData.rssrc/uws_sys/ListenSocket.rssrc/uws_sys/Loop.rssrc/uws_sys/SocketContext.rssrc/uws_sys/SocketGroup.rssrc/uws_sys/SocketKind.rssrc/uws_sys/Timer.rssrc/uws_sys/lib.rssrc/uws_sys/libuwsockets.cppsrc/uws_sys/socket.rssrc/uws_sys/udp.rssrc/uws_sys/us_socket_t.rssrc/uws_sys/vtable.rstest/js/bun/net/socket-syscall-fault.test.tstest/js/bun/net/socket.test.tstest/js/node/http/node-http-syscall-fault.test.tstest/js/node/net/connect-autoselectfamily-cancel-loop-fixture.jstest/js/node/net/connect-autoselectfamily-stale-timer.test.ts
💤 Files with no reviewable changes (19)
- src/install/Cargo.toml
- packages/bun-usockets/src/internal/eventing/libuv.h
- packages/bun-uws/src/AsyncSocket.h
- packages/bun-usockets/src/internal/eventing/epoll_kqueue.h
- packages/bun-usockets/src/context.c
- packages/bun-usockets/src/internal/loop_data.h
- packages/bun-usockets/src/internal/fault_inject.h
- packages/bun-usockets/src/fault_inject.c
- src/jsc/bindings/bindings.cpp
- packages/bun-usockets/src/crypto/sni_tree.cpp
- packages/bun-usockets/src/internal/networking/bsd.h
- packages/bun-usockets/src/eventing/libuv.c
- packages/bun-usockets/src/internal/internal.h
- packages/bun-usockets/src/loop.c
- packages/bun-usockets/src/eventing/epoll_kqueue.c
- packages/bun-usockets/src/udp.c
- packages/bun-usockets/src/bsd.c
- packages/bun-usockets/src/socket.c
- src/uws_sys/libuwsockets.cpp
🛑 Comments failed to post (4)
src/event_loop/SpawnSyncEventLoop.rs (1)
379-389: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Minor: reuse the already-extracted
uv_looppointer instead of recomputing it.
uv_loopis computed viaself.uws_loop().uv_loop.cast::<libuv::Loop>()on line 381 and then the same expression is recomputed on line 389 foruv_update_time. Reusing the local avoids a second&selfborrow/FFI-field read for the same value.♻️ Proposed dedup
let uv_loop = self.uws_loop().uv_loop.cast::<libuv::Loop>(); self.uv_timer_mut().expect("just set").init(uv_loop); + // Refresh the loop's cached clock using the pointer already extracted above. + unsafe { libuv::uv_update_time(uv_loop) }; + return; } - - // Refresh the loop's cached clock: ... - unsafe { libuv::uv_update_time(self.uws_loop().uv_loop.cast()) };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/event_loop/SpawnSyncEventLoop.rs` around lines 379 - 389, Reuse the existing uv_loop local from the timer initialization block when calling libuv::uv_update_time, instead of recomputing self.uws_loop().uv_loop.cast(). Keep the existing safety context and timer behavior unchanged.src/usockets/tls/context.rs (1)
143-175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
x509_error_codetable is duplicated verbatim instate.rs.This 30-entry X509_V_ERR → symbolic-name table is byte-for-byte identical to
x509_error_codeinsrc/usockets/tls/state.rs(Lines 1272-1309); the only difference is the return type (&'static CStrhere vs*const c_charthere). Two copies must be kept in sync when BoringSSL error codes change. Consider making thestate.rsvariant delegate to this one via.as_ptr()(or share a single table), keeping one source of truth.As per coding guidelines: "One source of truth ... Never copy a helper or constant table between modules — share or derive it."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/usockets/tls/context.rs` around lines 143 - 175, The X509 error-code mapping is duplicated between x509_error_code and the state.rs variant. Make the state.rs helper delegate to x509_error_code and convert its returned CStr with as_ptr(), preserving the existing state.rs return type and leaving a single mapping source of truth.Source: Coding guidelines
src/uws_shim/App.rs (1)
143-163: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
publish_with_optionsandpublishare byte-identical implementations.Both methods take the same parameters and call
c::uws_publishwith the same arguments (SSL as i32andSelf::SSL_FLAGare equal by definition). Per repo convention, duplicated logic like this should be collapsed — have one delegate to the other so future changes to the publish call don't silently diverge between the two entry points.♻️ Suggested dedup
pub fn publish_with_options( &mut self, topic: &[u8], message: &[u8], opcode: Opcode, compress: bool, ) -> SendStatus { - // SAFETY: self is a valid *mut uws_app_t; slices are valid for the call. - unsafe { - c::uws_publish( - SSL as i32, - std::ptr::from_mut::<Self>(self).cast::<uws_app_t>(), - topic.as_ptr(), - topic.len(), - message.as_ptr(), - message.len(), - opcode, - compress, - ) - } + self.publish(topic, message, opcode, compress) }Based on learnings, this repo's landing-PR guidance states: "If your fix makes two functions byte-identical, delete one."
Also applies to: 323-343
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/uws_shim/App.rs` around lines 143 - 163, Deduplicate the publish entry points: update either publish_with_options or publish to delegate to the other instead of independently calling c::uws_publish, while preserving the existing arguments and SendStatus behavior. Apply the same consolidation to the corresponding implementation around the second referenced method, and remove the now-redundant byte-identical logic.src/uws_shim/BodyReaderMixin.rs (1)
156-207: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Duplicated size-check/reserve/extend block between the "last, non-empty accumulator" and "non-last" branches.
The
MAX_BODY_SIZEcheck +try_reserve+extend_from_slicesequence (lines ~169-177 and ~195-204) is byte-for-byte identical logic repeated twice in the same function. Extracting a shared helper (e.g.fn append_chunk(body: &mut Vec<u8>, chunk: &[u8]) -> Result<(), bun_core::Error>) would remove the duplication and guard against the two copies drifting if the size/OOM policy changes later.♻️ Suggested extraction
+ fn append_chunk(body: &mut Vec<u8>, chunk: &[u8]) -> Result<(), bun_core::Error> { + if body.len().saturating_add(chunk.len()) > MAX_BODY_SIZE { + return Err(bun_core::err!(RequestBodyTooLarge)); + } + if body.try_reserve(chunk.len()).is_err() { + return Err(bun_core::err!(OutOfMemory)); + } + body.extend_from_slice(chunk); + Ok(()) + } + fn on_data(...) -> Result<(), bun_core::Error> { if last { let mut body = mem::take(&mut Self::mixin_of(wrap).body); resp.clear_on_data(); if !body.is_empty() { - if body.len().saturating_add(chunk.len()) > MAX_BODY_SIZE { - return Err(bun_core::err!(RequestBodyTooLarge)); - } - if body.try_reserve(chunk.len()).is_err() { - return Err(bun_core::err!(OutOfMemory)); - } - body.extend_from_slice(chunk); + Self::append_chunk(&mut body, chunk)?; unsafe { Wrap::on_body(wrap, body.as_slice(), resp)? }; } else { ... } Ok(()) } else { let body = &mut Self::mixin_of(wrap).body; - if body.len().saturating_add(chunk.len()) > MAX_BODY_SIZE { - return Err(bun_core::err!(RequestBodyTooLarge)); - } - if body.try_reserve(chunk.len()).is_err() { - return Err(bun_core::err!(OutOfMemory)); - } - body.extend_from_slice(chunk); - Ok(()) + Self::append_chunk(body, chunk) } }Based on learnings, the repo's landing-PR guidance states: "The second time a multi-line block appears in your diff, extract a named helper and use it at EVERY parallel site."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.fn append_chunk(body: &mut Vec<u8>, chunk: &[u8]) -> Result<(), bun_core::Error> { if body.len().saturating_add(chunk.len()) > MAX_BODY_SIZE { return Err(bun_core::err!(RequestBodyTooLarge)); } if body.try_reserve(chunk.len()).is_err() { return Err(bun_core::err!(OutOfMemory)); } body.extend_from_slice(chunk); Ok(()) } fn on_data( wrap: *mut Wrap, resp: AnyResponse, chunk: &[u8], last: bool, ) -> Result<(), bun_core::Error> { if last { // Free everything after. Take via the mixin field first — no // `&mut Wrap` is live yet, and the temporary `&mut Self` ends at // the `;` (before `on_body`, which may heap::take(wrap)). let mut body = mem::take(&mut Self::mixin_of(wrap).body); resp.clear_on_data(); if !body.is_empty() { Self::append_chunk(&mut body, chunk)?; // SAFETY: wrap is the original heap-allocated pointer; the &mut to // mixin.body has ended, so on_body receives sole ownership of the // allocation and may heap::take it on success. unsafe { Wrap::on_body(wrap, body.as_slice(), resp)? }; } else { if chunk.len() > MAX_BODY_SIZE { return Err(bun_core::err!(RequestBodyTooLarge)); } // SAFETY: wrap is the original heap-allocated pointer; the &mut to // mixin.body has ended, so on_body receives sole ownership of the // allocation and may heap::take it on success. unsafe { Wrap::on_body(wrap, chunk, resp)? }; } // `body` drops here Ok(()) } else { let body = &mut Self::mixin_of(wrap).body; Self::append_chunk(body, chunk) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/uws_shim/BodyReaderMixin.rs` around lines 156 - 207, Extract the duplicated body accumulation logic from on_data into a shared helper, such as append_chunk, that performs the MAX_BODY_SIZE check, try_reserve, and extend_from_slice operations and returns the existing errors. Replace both the non-last branch and the last branch’s non-empty accumulator sequence with calls to this helper, preserving their current ownership and response-handling behavior.
|
@robobun run |
Benchmark results: this branch vs canarySetup: Note on binariesNo linux-x64 artifact existed for this PR when I started (earlier builds failed at the cpp stage, #72235 was still building), so I built the PR locally with Same toolchain:
|
| Framework | Binary | req/s (best of 3) | p50 | p99 | RSS | Δ req/s |
|---|---|---|---|---|---|---|
| express | main | 41,369 | 2.24 ms | 4.65 ms | 92.8 MB | |
| PR | 41,596 | 2.21 ms | 4.77 ms | 91.5 MB | +0.5% | |
| fastify | main | 62,619 | 1.48 ms | 3.22 ms | 119.1 MB | |
| PR | 64,713 | 1.44 ms | 3.12 ms | 118.7 MB | +3.3% | |
| elysia | main | 138,480 | 0.67 ms | 1.50 ms | 60.5 MB | |
| PR | 142,101 | 0.66 ms | 1.41 ms | 60.1 MB | +2.6% |
With matched build flags the rewrite is at parity or slightly ahead on throughput, with marginally lower p50/p99 and RSS within noise. No regression.
As requested: published canary (LTO) vs local PR build (non-LTO)
| Framework | Binary | req/s (best of 3) | p50 | p99 | RSS | Δ req/s |
|---|---|---|---|---|---|---|
| express | canary 2e2230a81 |
47,769 | 1.92 ms | 4.16 ms | 89.3 MB | |
PR 76786b440 |
42,314 | 2.18 ms | 4.66 ms | 95.6 MB | -11.4% | |
| fastify | canary | 69,127 | 1.34 ms | 2.89 ms | 113.4 MB | |
| PR | 60,831 | 1.53 ms | 3.27 ms | 118.1 MB | -12.0% | |
| elysia | canary | 149,507 | 0.63 ms | 1.36 ms | 53.7 MB | |
| PR | 140,233 | 0.66 ms | 1.49 ms | 59.5 MB | -6.2% |
The 6-12% gap here lines up with the LTO vs non-LTO delta documented in scripts/build/profiles.ts (the btg profile comment). Once this PR has a linux-x64 CI artifact I can re-run against that for a true LTO-vs-LTO number.
Raw data (all 36 runs)
Same-toolchain (local release, non-LTO):
LABEL=main-express-r1 RPS=41369.34967077393 P50=0.002240064 P99=0.004653211 RSS_KB=95016 SUCCESS=1.0
LABEL=pr2-express-r1 RPS=41595.87266272149 P50=0.002206526 P99=0.004769115 RSS_KB=93712 SUCCESS=1.0
LABEL=main-express-r2 RPS=40558.2380264613 P50=0.002248189 P99=0.004840736 RSS_KB=97748 SUCCESS=1.0
LABEL=pr2-express-r2 RPS=40588.487870720666 P50=0.002218532 P99=0.004787946 RSS_KB=98228 SUCCESS=1.0
LABEL=main-express-r3 RPS=40164.99325194582 P50=0.002293524 P99=0.004891689 RSS_KB=93352 SUCCESS=1.0
LABEL=pr2-express-r3 RPS=41367.68204938798 P50=0.002215952 P99=0.004754929 RSS_KB=98820 SUCCESS=1.0
LABEL=main-fastify-r1 RPS=62619.23500251308 P50=0.001479851 P99=0.00321547 RSS_KB=121956 SUCCESS=1.0
LABEL=pr2-fastify-r1 RPS=64712.53706578089 P50=0.001439639 P99=0.00312319 RSS_KB=121516 SUCCESS=1.0
LABEL=main-fastify-r2 RPS=60741.39885998867 P50=0.001534144 P99=0.003306825 RSS_KB=120548 SUCCESS=1.0
LABEL=pr2-fastify-r2 RPS=63238.934082231615 P50=0.00146082 P99=0.003212499 RSS_KB=121804 SUCCESS=1.0
LABEL=main-fastify-r3 RPS=57989.158681876674 P50=0.001605662 P99=0.003391715 RSS_KB=119964 SUCCESS=1.0
LABEL=pr2-fastify-r3 RPS=55794.82431949291 P50=0.001649222 P99=0.003541728 RSS_KB=119828 SUCCESS=1.0
LABEL=main-elysia-r1 RPS=138170.75825080505 P50=0.000673225 P99=0.00148168 RSS_KB=61348 SUCCESS=1.0
LABEL=pr2-elysia-r1 RPS=141726.31520931728 P50=0.000661769 P99=0.001406469 RSS_KB=61920 SUCCESS=1.0
LABEL=main-elysia-r2 RPS=138479.84321339754 P50=0.000668068 P99=0.001496186 RSS_KB=61948 SUCCESS=1.0
LABEL=pr2-elysia-r2 RPS=139272.68822484228 P50=0.000659479 P99=0.001496964 RSS_KB=59524 SUCCESS=1.0
LABEL=main-elysia-r3 RPS=135120.06225991691 P50=0.000669057 P99=0.001626931 RSS_KB=62580 SUCCESS=1.0
LABEL=pr2-elysia-r3 RPS=142100.51216910593 P50=0.000659413 P99=0.001410317 RSS_KB=61576 SUCCESS=1.0
CI canary (LTO) vs local PR (non-LTO):
LABEL=canary-express-r1 RPS=47769.478677672785 P50=0.001924249 P99=0.004156298 RSS_KB=91468 SUCCESS=1.0
LABEL=pr-express-r1 RPS=42314.40891509191 P50=0.002183895 P99=0.004655112 RSS_KB=97860 SUCCESS=1.0
LABEL=canary-express-r2 RPS=46896.04703654922 P50=0.001955285 P99=0.004203238 RSS_KB=89096 SUCCESS=1.0
LABEL=pr-express-r2 RPS=35952.19977432369 P50=0.00250601 P99=0.005755512 RSS_KB=98060 SUCCESS=1.0
LABEL=canary-express-r3 RPS=43816.56441440306 P50=0.002062849 P99=0.004522195 RSS_KB=91944 SUCCESS=1.0
LABEL=pr-express-r3 RPS=40381.790012560625 P50=0.002268756 P99=0.004824348 RSS_KB=95704 SUCCESS=1.0
LABEL=canary-fastify-r1 RPS=67658.1510393544 P50=0.001371552 P99=0.00295735 RSS_KB=116208 SUCCESS=1.0
LABEL=pr-fastify-r1 RPS=59917.982527619446 P50=0.00154592 P99=0.003328061 RSS_KB=120548 SUCCESS=1.0
LABEL=canary-fastify-r2 RPS=66390.23170416798 P50=0.001384621 P99=0.003028206 RSS_KB=115768 SUCCESS=1.0
LABEL=pr-fastify-r2 RPS=60830.73210227568 P50=0.001525845 P99=0.00326506 RSS_KB=120980 SUCCESS=1.0
LABEL=canary-fastify-r3 RPS=69126.97604724011 P50=0.001344122 P99=0.00288599 RSS_KB=116132 SUCCESS=1.0
LABEL=pr-fastify-r3 RPS=55509.06511713213 P50=0.001651118 P99=0.00356525 RSS_KB=119540 SUCCESS=1.0
LABEL=canary-elysia-r1 RPS=149507.46952460633 P50=0.000625089 P99=0.001356977 RSS_KB=54940 SUCCESS=1.0
LABEL=pr-elysia-r1 RPS=137633.70679401132 P50=0.000667992 P99=0.001524142 RSS_KB=62760 SUCCESS=1.0
LABEL=canary-elysia-r2 RPS=146930.43791036378 P50=0.00062744 P99=0.001445462 RSS_KB=55240 SUCCESS=1.0
LABEL=pr-elysia-r2 RPS=137341.97083857734 P50=0.000653064 P99=0.001633563 RSS_KB=61788 SUCCESS=1.0
LABEL=canary-elysia-r3 RPS=146649.0477090695 P50=0.000635603 P99=0.001401454 RSS_KB=54712 SUCCESS=1.0
LABEL=pr-elysia-r3 RPS=140232.62799679633 P50=0.000659426 P99=0.001485341 RSS_KB=60896 SUCCESS=1.0
Server scripts:
// express.js
const app = require("express")();
app.get("/", (req, res) => res.send("Hello, World!"));
const server = app.listen(0, () => console.log("PORT=" + server.address().port));
// fastify.js
const fastify = require("fastify")({ logger: false });
fastify.get("/", async () => "Hello, World!");
fastify.listen({ port: 0, host: "127.0.0.1" }).then(() => console.log("PORT=" + fastify.server.address().port));
// elysia.ts
import { Elysia } from "elysia";
const app = new Elysia().get("/", () => "Hello, World!").listen(0);
console.log("PORT=" + app.server!.port);oha --no-tui -z 15s -c 100 --output-format json http://127.0.0.1:$PORT/
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/io/windows_event_loop.rs (1)
300-309: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winExport
Resultfrombun_ioor switch thisinit()tocrate::error::Result<Self>.src/io/error.rsdefinespub type Result<T, E = Error>, butsrc/io/lib.rsonly re-exportsError, socrate::Resultat this call site has no crate-root definition.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/io/windows_event_loop.rs` around lines 300 - 309, Update Waker::init to use the existing crate::error::Result<Self> alias, or re-export the io Result alias from bun_io before using crate::Result. Preserve the current infallible initialization behavior and Result<Waker, crate::Error> API contract.src/io/posix_event_loop.rs (1)
857-871: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClear
Flags::OneShotin theNeedsRearmskip path.register_with_fd_impl()never clears that bit forone_shot: false, so a later persistent re-register can still be handled as one-shot and get disarmed on the next event.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/io/posix_event_loop.rs` around lines 857 - 871, Update the NeedsRearm early-return path in unregister around register_with_fd_impl behavior to also remove Flags::OneShot before returning. Preserve the existing cleanup of the other polling flags and the skip behavior for non-forced unregisters.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/http/lib.rs`:
- Around line 1284-1289: Retain validate_request_target’s rejection of spaces,
control bytes, and DEL, and ensure it is applied to href/path/host values before
constructing request or CONNECT lines, including proxy paths. Preserve the
existing InvalidURL error behavior.
In `@src/io/lib.rs`:
- Around line 15-16: Update the exports in src/io/lib.rs alongside Error so the
Result type used by the crate::Result<Self> call site is available from the
crate root; re-export the existing error::Result rather than introducing a new
result type.
In `@src/jsc/event_loop.rs`:
- Around line 1127-1128: Remove the duplicate consecutive process_gc_timer call
in tick_possibly_forever, leaving a single invocation in the event-loop
processing sequence.
In `@src/jsc/ipc.rs`:
- Around line 758-760: Update owner_teardown and the related close paths around
the socket teardown logic so they do not call close_socket while holding
JsCell::with_mut through with_queue. Reuse the existing audited reentrant queue
projection helper consistently, allowing on_close to re-project the same queue
without violating the borrow scope.
- Around line 670-684: Condense the lifecycle comments around IpcData and the
additionally referenced sections near owner teardown and related callbacks to no
more than three lines each. Preserve only the essential ownership/refcount and
teardown invariant, removing extended protocol, path, and implementation
explanations while leaving code unchanged.
---
Outside diff comments:
In `@src/io/posix_event_loop.rs`:
- Around line 857-871: Update the NeedsRearm early-return path in unregister
around register_with_fd_impl behavior to also remove Flags::OneShot before
returning. Preserve the existing cleanup of the other polling flags and the skip
behavior for non-forced unregisters.
In `@src/io/windows_event_loop.rs`:
- Around line 300-309: Update Waker::init to use the existing
crate::error::Result<Self> alias, or re-export the io Result alias from bun_io
before using crate::Result. Preserve the current infallible initialization
behavior and Result<Waker, crate::Error> API contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 343983e5-c8f9-43ed-a2e5-8d44f9937cc5
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
scripts/build/bun.tsscripts/build/source.tssrc/http/Cargo.tomlsrc/http/HTTPContext.rssrc/http/HTTPThread.rssrc/http/ProxyTunnel.rssrc/http/error.rssrc/http/h2_client/ClientSession.rssrc/http/h3_client/ClientContext.rssrc/http/h3_client/ClientSession.rssrc/http/h3_client/PendingConnect.rssrc/http/h3_client/callbacks.rssrc/http/h3_client/encode.rssrc/http/lib.rssrc/http/ssl_config.rssrc/http_jsc/websocket_client.rssrc/http_jsc/websocket_client/WebSocketProxyTunnel.rssrc/http_jsc/websocket_client/WebSocketUpgradeClient.rssrc/install/Cargo.tomlsrc/install/PackageManager/security_scanner.rssrc/install/lifecycle_script_runner.rssrc/install/resolution.rssrc/io/Cargo.tomlsrc/io/PipeReader.rssrc/io/PipeWriter.rssrc/io/lib.rssrc/io/posix_event_loop.rssrc/io/windows_event_loop.rssrc/jsc/Cargo.tomlsrc/jsc/Debugger.rssrc/jsc/VirtualMachine.rssrc/jsc/error.rssrc/jsc/event_loop.rssrc/jsc/ipc.rssrc/jsc/lib.rssrc/jsc/rare_data.rssrc/jsc/web_worker.rs
💤 Files with no reviewable changes (1)
- src/install/resolution.rs
Ceiling benchmark (LTO vs LTO)Binaries: published canary Setup:
Notes:
Raw data (all 48 runs)Server scripts: // hello.js
const server = Bun.serve({ port: 0, fetch() { return new Response("hello"); } });
console.log("PORT=" + server.port);
// routes.js (note: (globalThis.i ??= 0)++ is a syntax error; rewritten)
globalThis.i = 0;
const server = Bun.serve({
port: 0,
routes: {
"/static": new Response("hello"),
"/dynamic": () => new Response("hii" + globalThis.i++),
},
});
console.log("PORT=" + server.port);
|
…tener-close deref, review findings - send_sync no longer ManuallyDrop-leaks response metadata (LSan exit-134 on bun info/audit under the asan lane); SyncHTTPResponse owns the metadata and exposes it through a lifetime-tied accessor so header slices cannot outlive the backing buffer - the intentionally-crashing native-plugin fixture disables crash reporting so its late-arriving SIGSEGV report can no longer be attributed to whatever test runs next (the fetch.stream / net-write-slow 'crashes') - http-chunked-server fixture: listen() before printing the port (RST window caused spurious ConnectionRefused) - SNI select_cert fallthrough re-reads the listener backref after user JS (sync resolver closing the listener hit a nulled ext) - connecting-socket unlink keeps links intact (C parity for re-entrant close_all walks); SNI label collection is allocation-free again; stale feature-gate comment
| "src/shell_parser", | ||
| "src/tcc_sys", | ||
| "src/uws_sys", | ||
| "src/usockets", |
There was a problem hiding this comment.
🟡 🟡 Three more stale references to files this PR deletes, in files the earlier grep-and-update passes missed because two of them are outside the diff: src/platform/linux.rs:13-14 ("The C caller in epoll_kqueue.c decodes errno from the return value" — epoll_kqueue.c is deleted; the caller is now src/usockets/unsafe_core/poll_access.rs), src/js/internal-for-testing.ts:280 ("The syscalls instrumented in bsd.c" — bsd.c is deleted; fault injection is now src/usockets/fault.rs), and src/usockets/kind.rs:2 ("identical to src/uws_sys/SocketKind.rs" — that file is deleted here). Same class as the already-fixed lib.rs/cabi.rs/README.md rounds; no runtime effect. (Filed against Cargo.toml because platform/linux.rs and internal-for-testing.ts have no diff line ranges.)
Extended reasoning...
What the issue is
Three doc comments describe a live relationship with files this PR deletes:
-
src/platform/linux.rs:13-14— the doc comment onraw_syscall6(which backssys_epoll_pwait2) reads: "The C caller inepoll_kqueue.cdecodes errno from the return value (ret == -EINTR,ret != -ENOSYS), so the in-band encoding is what it expects." This PR deletespackages/bun-usockets/src/eventing/epoll_kqueue.c; the sole remaining caller issrc/usockets/unsafe_core/poll_access.rs. The comment describes a caller contract for a caller that no longer exists. -
src/js/internal-for-testing.ts:280— the JSDoc onSocketFaultSyscallreads: "The syscalls instrumented in bsd.c". This PR deletespackages/bun-usockets/src/bsd.candfault_inject.c; fault injection now lives insrc/usockets/fault.rs. -
src/usockets/kind.rs:2— reads: "Discriminants FROZEN — identical tosrc/uws_sys/SocketKind.rs".ls src/uws_sys/shows only_libusockets.h,libuwsockets.cpp,libuwsockets_h3.cpp—SocketKind.rsis deleted by this same PR.
Why the earlier passes missed them
This is the same finding class as the previously-accepted-and-fixed nits on this PR: #4 (lib.rs:12-14 + cabi.rs:12-16 → SocketKind.rs / "live C"), #8 (README.md → epoll_kqueue.c), and #13 (lib.rs:40 → "feature-gated OFF"). Those fix passes covered files the PR touches. The first two files here — src/platform/linux.rs and src/js/internal-for-testing.ts — are not in the PR's changed-files list, so a grep-and-update sweep over the diff would not find them. The third (kind.rs) is a new file added by this PR whose module doc references a file deleted in the same PR.
Impact
No runtime effect — pure doc staleness, hence nit. The platform/linux.rs one is the most misleading of the three: it documents an errno-in-band caller contract ("the C caller decodes errno from the return value") for a caller that no longer exists, so a reader trying to understand why the function returns raw -errno instead of setting thread-local errno would look for epoll_kqueue.c and find nothing. Per CLAUDE.md "Comments carry only durable non-obvious content" and "grep the whole repo including cfg-gated code" when a change invalidates surrounding docs.
Step-by-step proof
sed -n '13,14p' src/platform/linux.rs→ "The C caller inepoll_kqueue.cdecodes errno from the return value (ret == -EINTR,ret != -ENOSYS)".ls packages/bun-usockets/src/eventing/→ No such file or directory. This PR deletesepoll_kqueue.c.rg sys_epoll_pwait2 src/usockets/→unsafe_core/poll_access.rs— the new (and only) caller.sed -n '280p' src/js/internal-for-testing.ts→ "The syscalls instrumented in bsd.c".ls packages/bun-usockets/src/bsd.c→ No such file or directory. This PR deletes it; fault injection is nowsrc/usockets/fault.rs.sed -n '2p' src/usockets/kind.rs→ "identical tosrc/uws_sys/SocketKind.rs".ls src/uws_sys/→_libusockets.h libuwsockets.cpp libuwsockets_h3.cpp. NoSocketKind.rs— deleted by this PR.- Neither
src/platform/linux.rsnorsrc/js/internal-for-testing.tsappears in this PR's changed-files list — hence why the earlier fix passes over touched files missed them.
How to fix
- linux.rs:13-14: replace "The C caller in
epoll_kqueue.c" with "The caller insrc/usockets/unsafe_core/poll_access.rs" (or just "The caller"), keeping the errno-in-band contract description. - internal-for-testing.ts:280: replace "instrumented in bsd.c" with "instrumented in
src/usockets/fault.rs". - kind.rs:2: drop "identical to
src/uws_sys/SocketKind.rs" — the docs/cabi.md §3.8 reference on the same line already covers the freeze contract, andSocketKind.rsno longer exists to be identical to.
(Filed against Cargo.toml because platform/linux.rs and internal-for-testing.ts have no diff line ranges in this PR.)
| // bun_uws_shim::ResponseKind, not the shim's: consumed only by unmigrated | ||
| // bun_uws-typed FFI (`FetchHeaders::to_uws_response`, `CookieMap::write`). |
There was a problem hiding this comment.
🟡 Companion to unresolved comment #16 (which covers deleted C-filename references): ~10 more comments still reference the deleted bun_uws / bun_uws_sys Rust crates or src/uws_sys/SocketKind.rs, which a C-filename grep pass would not find. Two are outright self-contradictory after a mechanical rename (bun_uws_shim::ResponseKind, not the shim's — bun_uws_shim IS the shim); three are in packages/bun-uws/*.h, a directory the earlier src/-scoped grep passes don't reach. All comment-only, no runtime effect. One rg 'bun_uws\b|bun_uws_sys|uws_sys/SocketKind' pass over comments would catch them — full site list in the details.
Extended reasoning...
What the issue is
This PR deletes the bun_uws and bun_uws_sys crates (removed from Cargo.toml members and workspace.dependencies) and src/uws_sys/SocketKind.rs (only _libusockets.h / libuwsockets.cpp / libuwsockets_h3.cpp survive in src/uws_sys/), but ~10 comments still reference them. The still-open comment #16 covers references to the deleted C filenames (epoll_kqueue.c, bsd.c, SocketKind.rs in kind.rs:2); this is the companion set — references to the deleted Rust crate names — which a C-filename grep would not find:
(a) Self-contradictory after a mechanical bun_uws:: → bun_uws_shim:: rename (both are + lines added by this PR):
src/runtime/server/RequestContext.rs:554-555: "bun_uws_shim::ResponseKind, not the shim's: consumed only by unmigrated bun_uws-typed FFI" — butbun_uws_shimIS the shim, so "not the shim's" is meaningless. Evidently written as "bun_uws::ResponseKind, not the shim's" during the intermediate two-crate state, then hit by a find-replace pass.src/runtime/server/server_body.rs:37-38: "ResponseKind stays bun_uws-typed" immediately aboveuse bun_uws_shim::ResponseKind;.
(b) Development-phase / re-export-chain comments in NEW uws_shim files referencing the deleted crates:
src/uws_shim/us_socket.rs:88-89: "Also#[no_mangle]-defined insrc/uws_sysuntil D1 deletes that crate; the two crates must never both link" — D1 already happened (same pattern as the fixedcabi.rs:12-16finding).src/uws_shim/Response.rs:25-27: "Higher tiers (bun_uws,bun_runtime) re-export this aspub use bun_uws_sys::SocketAddress" — both crates deleted; no such re-export chain.src/uws_shim/Response.rs:63/65/67: three doc comments say "concrete type lives inbun_uws" — deleted; these are opaque handles for the C++ uWS layer.src/sourcemap/Mapping.rs:107: "Mirrorsany_dispatch!atsrc/uws_sys/Response.rs:581" — file deleted; the macro now lives insrc/uws_shim/Response.rs.
(c) Surviving C++ headers pointing at src/uws_sys/SocketKind.rs (deleted; the BUN_SOCKET_KIND_* #[no_mangle] statics now live in src/usockets/cabi.rs):
packages/bun-uws/src/SocketKinds.h:2: "src/uws_sys/SocketKind.rsis the source of truth for these ordinals"packages/bun-uws/src/HttpContext.h:128andWebSocketContext.h:53: "the ordinals are linked fromsrc/uws_sys/SocketKind.rs"
Why the earlier passes missed them
Rounds #4/#8/#13 and the still-open #16 covered stale references at lib.rs/cabi.rs/README.md/platform/linux.rs/internal-for-testing.ts/kind.rs — found by grepping for the deleted C filenames and by touching files inside src/. Group (a)/(b) reference the deleted Rust crate names instead. Group (c) is under packages/bun-uws/, which the src/-scoped sweeps don't reach — even though HttpContext.h is in this PR's changed-files list (the author was already editing it).
Impact
No runtime effect — pure comment staleness, hence nit. The two group-(a) comments are actively misleading (self-contradictory nonsense), the group-(b) "D1" / re-export-chain comments describe a development phase that no longer exists, and the group-(c) headers point future readers at the wrong file for the ABI ordinal source of truth. Per CLAUDE.md "Comments carry only durable non-obvious content" and "Delete dead code in the same PR that makes it dead".
Step-by-step proof
Cargo.tomldiff removes"src/uws"frommembersandbun_uws = { path = "src/uws" }fromworkspace.dependencies; addsbun_uws_shim. Sobun_uwsis deleted andbun_uws_shimIS the shim crate.RequestContext.rs:554(+line): "bun_uws_shim::ResponseKind, not the shim's" — literally self-contradictory given (1).server_body.rs:37-39: comment says "stays bun_uws-typed", import right below isbun_uws_shim::ResponseKind.ls src/uws_sys/→_libusockets.h libuwsockets.cpp libuwsockets_h3.cpp. No.rsfiles. Sous_socket.rs:88-89("until D1 deletes that crate"),Response.rs:25-27("pub use bun_uws_sys::SocketAddress"),Response.rs:63/65/67("lives inbun_uws"), andMapping.rs:107("src/uws_sys/Response.rs:581") all reference deleted files/crates.rg 'uws_sys/SocketKind' packages/bun-uws/src→ three hits at exactlySocketKinds.h:2,HttpContext.h:128,WebSocketContext.h:53.rg 'BUN_SOCKET_KIND_' src/usockets/cabi.rs→ the#[no_mangle]statics live there now.- None of these locations overlap the still-open Fix
browsermap #16 (platform/linux.rs,internal-for-testing.ts,kind.rs:2) or the previously-fixed rounds — distinct sites, same class.
How to fix
One rg 'bun_uws\b|bun_uws_sys|uws_sys/SocketKind' pass over comments:
- (a): delete both comments outright — after the migration there is only one
ResponseKind, so the distinction they try to draw doesn't exist. - (b): delete
us_socket.rs:88-89andResponse.rs:25-27; changeResponse.rs:63/65/67to reference the C++ uWS layer; repointMapping.rs:107atsrc/uws_shim/Response.rs(or drop the cross-reference). - (c): point all three headers at
src/usockets/cabi.rs(where theBUN_SOCKET_KIND_*#[no_mangle]exports live) orsrc/usockets/kind.rs(the enum definition).
(Filed at RequestContext.rs:554 — the one site that has a commentable diff line range; the new uws_shim/*.rs files and the packages/bun-uws/*.h lines have none.)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/usockets/tls/sni.rs`:
- Around line 151-161: Update the SNI pattern insertion method containing
collect_labels and remove_rec so it returns immediately when collect_labels
rejects a pattern, before any trie mutation or SslCtx reference retention
occurs. Ensure over-limit patterns are not stored, while valid patterns continue
through the existing add flow.
- Around line 159-161: Update the lookup in find_ctx to use the exact-match
traversal rather than get(), so querying a concrete hostname does not resolve
wildcard children. Preserve the existing label collection and return the exact
context when the full pattern exists, otherwise return null.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 15ee566c-9713-4758-89fa-c5e06311252d
📒 Files selected for processing (15)
scripts/rust-miri.tssrc/http/AsyncHTTP.rssrc/install/npm.rssrc/runtime/cli/audit_command.rssrc/runtime/cli/create_command.rssrc/runtime/cli/pm_view_command.rssrc/runtime/cli/publish_command.rssrc/runtime/cli/upgrade_command.rssrc/standalone_graph/StandaloneModuleGraph.rssrc/usockets/group.rssrc/usockets/lib.rssrc/usockets/tls/sni.rssrc/usockets/unsafe_core/ffi.rstest/bundler/native-plugin.test.tstest/js/web/fetch/http-chunked-server.c
| let mut buf: [&[u8]; MAX_LABELS] = [b""; MAX_LABELS]; | ||
| let Some(n) = collect_labels(pattern.to_bytes(), &mut buf) else { | ||
| return; | ||
| }; | ||
| drop(remove_rec(&mut self.root, &buf[..n])); | ||
| } | ||
|
|
||
| fn lookup(&self, hostname: &CStr) -> Option<&Entry> { | ||
| let mut buf: [&[u8]; MAX_LABELS] = [b""; MAX_LABELS]; | ||
| let n = collect_labels(hostname.to_bytes(), &mut buf)?; | ||
| get(&self.root, &buf[..n]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject over-limit SNI patterns before storing them.
collect_labels() rejects patterns with more than MAX_LABELS labels, but add() still accepts and stores them. Such entries become unreachable to lookup and removal, while their SslCtx up-reference remains held. Apply the same limit before mutating the trie.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/usockets/tls/sni.rs` around lines 151 - 161, Update the SNI pattern
insertion method containing collect_labels and remove_rec so it returns
immediately when collect_labels rejects a pattern, before any trie mutation or
SslCtx reference retention occurs. Ensure over-limit patterns are not stored,
while valid patterns continue through the existing add flow.
| let mut buf: [&[u8]; MAX_LABELS] = [b""; MAX_LABELS]; | ||
| let n = collect_labels(hostname.to_bytes(), &mut buf)?; | ||
| get(&self.root, &buf[..n]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep exact find_ctx lookups separate from wildcard resolution.
The lookup path uses get(), which falls back to * children, but find_ctx() is documented as exact-pattern lookup. If only *.example.com exists, querying api.example.com returns the wildcard context instead of null.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/usockets/tls/sni.rs` around lines 159 - 161, Update the lookup in
find_ctx to use the exact-match traversal rather than get(), so querying a
concrete hostname does not resolve wildcard children. Preserve the existing
label collection and return the exact context when the full pattern exists,
otherwise return null.
Core: clamp expired tick timespecs to immediate-return (spawnSync could park forever past its deadline); close_all walks restart from the head after every dispatch instead of trusting cached next pointers (re-entrant adopt/close could route the walk into a foreign group or exit early, leaving parked sockets with dangling group pointers); nested-tick depth bracketing keeps slots undrained across close_all frames; kqueue poll_stop deletes an armed one-shot WRITE knote on every fd-outlives-poll path; happy-eyeballs attempt sockets close through the connecting cancel path; poll-registry fds are range-checked against the 27-bit poll field; field-granular raw-place access replaces whole-struct &mut Loop on paths foreign threads touch concurrently. node:net: partial-writev remainder used swapped operands, duplicating or dropping bytes under backpressure (inherited from the pre-rewrite implementation; regression test fails on released bun); fully-written buffered prefixes are now cleared; bytes_written accounting added; detached handles are no longer resurrected by late TLS events; upgradeTLS twins are retired on reconnect/close/deinit paths. IPC: NODE_HANDLE retransmission counter resets on ack; partial sendmsg retries no longer resend the SCM_RIGHTS fd; oversized JSON-mode messages error instead of panicking; nested on_end uses the raw-place projection. HTTP thread: exit-path context drops deferred until the loop parks; queued writes for pending h2 attaches are held, not discarded.
| "src/shell_parser", | ||
| "src/tcc_sys", | ||
| "src/uws_sys", | ||
| "src/usockets", |
There was a problem hiding this comment.
🟡 The doc comments at src/usockets/tls/sni.rs:164 ("Exact-pattern lookup") and src/usockets/unsafe_core/ffi.rs:714 ("exact-pattern SNI lookup") are wrong — find_ctx() → lookup() → get() falls back to * children (sni.rs:107), which is wildcard-aware and matches the deleted C (sni_find → getUser). The unresolved CodeRabbit comment at sni.rs:161 suggests the opposite fix (change the impl to exact-match), but that would regress App.h onMissingServerName — which passes the concrete negotiated hostname after user JS may have registered *.example.com, and needs the wildcard match. Fix the two doc comments; do not apply the CodeRabbit suggestion. (Filed against Cargo.toml because the new sni.rs file has no commentable diff line ranges.)
Extended reasoning...
What the issue is
Two doc comments describe find_ctx() as an exact-pattern lookup, but the implementation is (correctly) wildcard-aware:
src/usockets/tls/sni.rs:164:/// Exact-pattern lookup; returns an OWNED reference (caller unrefs)src/usockets/unsafe_core/ffi.rs:714:/// `us_listen_socket_find_server_name_ctx`: exact-pattern SNI lookup
But find_ctx() (sni.rs:166) calls self.lookup() (sni.rs:158), which calls get() (sni.rs:98–108), and get() falls back to the * child at sni.rs:107 (get(node.children.get(&b"*"[..])?, rest)) — the same wildcard-aware traversal that resolve() (sni.rs:184, correctly documented as "Wildcard-aware") uses. So the two doc comments contradict the implementation.
Why the implementation is correct (C-parity)
The deleted C function this ports is us_listen_socket_find_server_name_ctx (openssl.c:2506–2513), which called sni_find. From the deleted sni_tree.cpp in this PR's diff, sni_find called getUser(root, 0, labels, numLabels), and getUser (sni_tree.cpp:92–116) has explicit wildcard fallback:
/* Try and match by wildcard */
it = root->children.find("*");
if (it == root->children.end()) {
return nullptr;
}
return getUser(it->second.get(), label + 1, labels, numLabels);So the Rust impl exactly matches the deleted C — the doc comments are what's wrong.
Why the CodeRabbit suggestion should NOT be applied
There is an unresolved CodeRabbit inline comment at sni.rs:161 (dated 2026-07-12T23:45:03Z) suggesting the opposite fix: "Update the lookup in find_ctx to use the exact-match traversal rather than get(), so querying a concrete hostname does not resolve wildcard children."
Applying that would be a regression. The sole caller of us_listen_socket_find_server_name_ctx is App.h:325 inside onMissingServerName:
static struct ssl_ctx_st *onMissingServerName(..., const char *hostname, ...) {
...
/* The handler is expected to have registered the name via addServerName();
* hand the newly-registered context back so the in-flight handshake uses it */
return us_listen_socket_find_server_name_ctx(ls, hostname);
}Here hostname is the concrete negotiated servername from the ClientHello (e.g. foo.example.com). The user's missingServerName handler may have just called addServerName("*.example.com", ...). With wildcard matching (current behavior, C-parity), find_server_name_ctx(ls, "foo.example.com") returns the newly-registered *.example.com context and the in-flight handshake uses it. With CodeRabbit's exact-match fix, it would return null and the handshake would fall back to the default context — breaking the wildcard-registration flow that the C supported.
Impact
No runtime effect — the implementation is correct today; only the two doc comments are wrong. Hence nit. But the finding's real value is flagging that the still-open CodeRabbit suggestion is backwards: applying it (which the "🤖 Prompt for AI Agents" block invites) would silently regress a user-visible flow that the deleted C supported and that has no test coverage in this PR.
Step-by-step proof
sni.rs:164: doc says "Exact-pattern lookup".sni.rs:166–167:find_ctx()callsself.lookup(pattern).sni.rs:158–162:lookup()callsget(&self.root, &buf[..n]).sni.rs:107:get()falls back tonode.children.get(&b"*"[..])— wildcard-aware, not exact.sni.rs:181–185:resolve()uses the SAMElookup()and is correctly documented "Wildcard-aware".- Deleted
sni_tree.cppin the diff:sni_find→getUser, which doesroot->children.find("*")fallback — the C was wildcard-aware. App.h:313–325:onMissingServerNamecallsus_listen_socket_find_server_name_ctx(ls, hostname)with the concrete negotiated hostname; the comment at 320–323 says the handler registered a name viaaddServerName()— which accepts wildcard patterns. Wildcard matching is required for a*.example.comregistration to satisfy afoo.example.comrequest.- Therefore: implementation is C-parity correct; the two doc comments are misleading; the CodeRabbit suggestion at sni.rs:161 would regress from the deleted C's behavior.
How to fix
- sni.rs:164: change to
/// Wildcard-aware lookup (same traversal asresolve); returns an OWNED reference (caller unrefs) — docs/cabi.md §1.6. - ffi.rs:714: drop "exact-pattern" — e.g.
/// `us_listen_socket_find_server_name_ctx`: wildcard-aware SNI lookup - Do not apply the CodeRabbit "exact-match traversal" suggestion at sni.rs:161.
|
fix the merge conflicts |
|
Merged main (ab20e85). Two conflicts resolved:
|
|
Current head's only real reds are the two connect-memleak GC tests. Verification on this exact tree: debug 8/8, debug with CI env ( @robobun retry the failed jobs |
|
Retriggered CI. |
|
Pattern analysis across the last 7 builds: the recurring failures have converged to the connect-memleak pair (+ occasionally another allocation-heavy test like no-orphans), always on the last-finishing linux lanes, always with zero output captured — the process dies without printing an assertion failure. Combined with the reproduction record (18/18 on this tree across debug/CI-env/fresh-release, plus the exact CI artifact passing 18/18 in an emulated Alpine arm64 container), the failure signature fits the host OOM killer on memory-pressured runners: deliberately-allocating GC tests, SIGKILLed silently, only on lanes that inherit hosts already loaded by earlier jobs. That's checkable on the CI side via @robobun retry the failed jobs |
|
Retriggered CI. |
…s linguist-generated src/bssl (bun_bssl) re-exports the vendored bssl-sys and hosts the committed per-target bindings; bun_usockets consumes it, and bun_boringssl_sys can migrate onto it as a follow-up. The generated wrapper files no longer count toward the diff on GitHub.
| `bun_bssl` (`src/bssl`, the tree's BoringSSL binding surface — consumed by | ||
| `bun_usockets`) re-exports the vendored `bssl-sys` crate | ||
| (`vendor/boringssl/rust/bssl-sys`) as the raw BoringSSL layer | ||
| (.rewrite-specs/tls-semantics.md Part 2d; api.md CHANGES 1). Upstream generates |
There was a problem hiding this comment.
🟡 The new committed src/bssl/bindings/README.md cites .rewrite-specs/tls-semantics.md at line 6 ("(.rewrite-specs/tls-semantics.md Part 2d; api.md CHANGES 1)") and again at line 41 ("Decision (tls-semantics.md OQ-4/OQ-5)") — but this same PR adds .rewrite-specs/ to .gitignore (below the existing /specs/ entry commented "Kept locally for the ongoing work; not part of the source tree"), so no reader will ever have that file. Same class as the already-fixed stale-comment rounds (#4/#8/#13) and the still-open #16/#17, but a distinct pattern (gitignored working notes, not deleted files) their grep patterns wouldn't find. No runtime effect; drop both parentheticals — the sentences read fine without them.
Extended reasoning...
What the issue is
src/bssl/bindings/README.md is a new file added by this PR (per the diff and .gitattributes hunk that references it). Two lines cite a working-notes file that this same PR gitignores:
- Line 6:
(.rewrite-specs/tls-semantics.md Part 2d; api.md CHANGES 1)— a parenthetical citation appended to the sentence describing whatbun_bsslre-exports. - Line 41:
Decision (tls-semantics.md OQ-4/OQ-5): bindings are vendored outputs rather than a build-time bindgen step— citing open-question numbers from the same working-notes file.
Meanwhile, this PR's .gitignore hunk adds .rewrite-specs/ immediately below the existing /specs/ entry, which carries the comment "Kept locally for the ongoing work; not part of the source tree":
# Web Streams rewrite working notes (design docs, spec transcription, review logs).
# Kept locally for the ongoing work; not part of the source tree.
/specs/
+.rewrite-specs/So a committed README references a working-notes file that (a) does not exist in the repo (ls .rewrite-specs/ → No such file or directory), (b) is explicitly gitignored by this same PR, and (c) no reader — reviewer or future maintainer — will ever have.
Why the earlier fix passes missed it
This PR has already accepted and fixed ~5 rounds of the same "development-history narration in committed docs" class: #4 (lib.rs:12-14 / cabi.rs:12-16), #8 (event_loop/README.md → epoll_kqueue.c), #13 (lib.rs:40 → "feature-gated OFF"), and the still-open #16 (deleted C filenames: epoll_kqueue.c, bsd.c) / #17 (deleted Rust crate names: bun_uws, bun_uws_sys, uws_sys/SocketKind). But this is a distinct pattern: not a reference to a deleted file but to a gitignored local working-notes directory that this PR itself gitignores. The grep patterns from #16 (epoll_kqueue|bsd\.c) and #17 (bun_uws\b|bun_uws_sys|uws_sys/SocketKind) would not match rewrite-specs or tls-semantics. Verified: rg 'rewrite-specs' src/ packages/ → single hit at README.md:6; rg 'tls-semantics' src/ → hits at README.md:6 and README.md:41 only.
Impact
No runtime effect — pure doc staleness, hence nit. The concrete downside: a reader trying to understand why bun_bssl was structured this way ("why vendored bindings instead of build-time bindgen?") is told the rationale lives in .rewrite-specs/tls-semantics.md Part 2d and OQ-4/OQ-5 — a file they cannot find. Per CLAUDE.md "Comments carry only durable non-obvious content: … no bug history — that belongs in the PR description", these citations are development-phase narration that belongs in the PR description (which already covers this decision under "Approach"), not in-tree.
Step-by-step proof
sed -n '6p' src/bssl/bindings/README.md→(.rewrite-specs/tls-semantics.md Part 2d; api.md CHANGES 1). Upstream generatessed -n '41p' src/bssl/bindings/README.md→Decision (tls-semantics.md OQ-4/OQ-5): bindings are vendored outputs rather- This PR's
.gitignorediff:+.rewrite-specs/at line 219, immediately below the/specs/entry commented "Kept locally for the ongoing work; not part of the source tree". ls .rewrite-specs/→No such file or directory— the directory does not exist in the checkout.rg 'rewrite-specs|tls-semantics' src/ packages/→ onlysrc/bssl/bindings/README.md(lines 6 and 41). No other file references it.- The grep patterns from open comments Fix
browsermap #16 (epoll_kqueue.c,bsd.c,SocketKind.rs) and Fix JSX parser bug:// commentafter tagName and before closing tag is broken #17 (bun_uws\b|bun_uws_sys|uws_sys/SocketKind) do not match either string, so this is not a duplicate.
How to fix
Drop both parentheticals — the sentences read fine without them:
- Line 6:
(vendor/boringssl/rust/bssl-sys) as the raw BoringSSL layer. Upstream generates(delete(.rewrite-specs/tls-semantics.md Part 2d; api.md CHANGES 1)). - Line 41:
Bindings are vendored outputs rather than a build-time bindgen step(deleteDecision (tls-semantics.md OQ-4/OQ-5):prefix, capitalize "bindings").
The actual rationale ("no bindgen/libclang toolchain dependency in the build, and the committed diff makes fork drift reviewable on bumps") is already stated in the same paragraph, so nothing is lost.
| wrap_c="$(mktemp --suffix=.c)" | ||
| (cd "$BORINGSSL" && bindgen rust/bssl-sys/wrapper.h \ | ||
| -o "$OUT_DIR/wrapper_$triple.rs" \ | ||
| "${BINDGEN_ARGS[@]}" \ | ||
| --wrap-static-fns-path "$wrap_c" \ | ||
| -- -I "$BORINGSSL/include" --target="$triple" "$@") | ||
| sed -i "1i $STAMP" "$OUT_DIR/wrapper_$triple.rs" |
There was a problem hiding this comment.
🟡 regenerate.sh uses GNU-only syntax that fails on stock macOS: line 52 mktemp --suffix=.c (BSD mktemp has no --suffix) and line 58 sed -i "1i $STAMP" (BSD sed needs -i '' and 1i\<newline>text). The script's defaults strongly imply a Linux host, but the header doesn't say so and README.md tells maintainers to fold this into the upgrade-boringssl flow — worth either a "# run on Linux (GNU coreutils/sed)" line in the header, or the portable rewrites (mktemp + mv for the suffix; printf + cat > tmp && mv instead of sed -i 1i).
Extended reasoning...
What the issue is
Two GNU-coreutils-only invocations in src/bssl/bindings/regenerate.sh fail on stock macOS/BSD:
- Line 52:
wrap_c="$(mktemp --suffix=.c)"— BSD/macOSmktemphas no--suffixoption (only-t prefixand a template argument). Fails withmktemp: illegal option -- -on the very firstgen()call. - Line 58:
sed -i "1i $STAMP" ...— two GNU-isms: bare-i(BSD sed requires an explicit backup-extension argument, so it consumes"1i $STAMP"as the backup suffix and then errors on the filename-as-script), and the one-lineNi textinsert form (BSD requires1i\<newline>text).
The script header (lines 1-10) documents required tools (bindgen-cli, vendor/boringssl, vendor/zig, MACOS_SDK, WINSYSROOT) but does not state a Linux-only host requirement, and README.md says regeneration "Must be re-run on every BoringSSL commit bump … fold into the upgrade-boringssl flow" — a workflow a macOS maintainer may run.
Addressing the refutation
One verifier argued this is intentional/below-threshold because the script's defaults (WINSYSROOT=/opt/winsysroot, MACOS_SDK=$HOME/.bun/build-cache/MacOSX*.sdk, hermetic zig libc headers with -nostdinc) unambiguously target a Linux cross-generation host, and a macOS user hitting mktemp: illegal option would immediately know to run on Linux or brew install coreutils gnu-sed.
That's a fair read of the design intent, and it's exactly why this is nit severity, not normal. But three points keep it above the filing threshold:
- The intent is undocumented. The header lists five prerequisites and two env overrides but never says "run on Linux".
mktemp: illegal option -- -is not self-explanatory — it says nothing about GNU vs BSD, and a maintainer following README.md's instruction to "re-run on every BoringSSL bump" wastes a debugging cycle before discovering the host constraint. - The MACOS_SDK default is ambiguous, not dispositive.
$HOME/.bun/build-cache/is where Bun's build tooling caches artifacts on every platform, including macOS — a macOS dev who has runbun bdmay well have an SDK there. It reads as "cross-gen from Linux" only if you already know that's the intent. - This PR has already accepted ~15 nits of exactly this class (stale comments, wrong hand-coded constants, undocumented host assumptions in maintainer-only scripts). The bar this PR's review has established is "if a one-line fix removes a maintainer papercut, it's worth flagging" — and this is a one-line fix.
The refutation's point about sort -V (line 30) being another GNU-ism is weaker than stated: macOS sort has supported -V since 10.15 and FreeBSD since 12.0, so it doesn't add to the case.
Why nothing prevents it
set -euo pipefail (line 11) means the script exits loudly on the first failing command — so there is zero risk of committing corrupted output. That's precisely why this is a nit and not a normal-severity finding: the failure mode is a cryptic error, not silent bad bindings.
Impact
Maintainer-only, run rarely (once per BoringSSL bump), outputs pre-committed. Nothing user-facing. The only cost is a macOS maintainer following README.md's instruction, hitting mktemp: illegal option -- -, and having to reverse-engineer that the script assumes GNU coreutils.
Step-by-step proof
- On stock macOS (any version — Apple ships BSD mktemp/sed, not GNU):
cd src/bssl/bindings && ./regenerate.sh. - Line 52 executes for the first target (
x86_64-unknown-linux-gnu):mktemp --suffix=.c. - BSD
mktempparses--suffix=.c: sees-, then-again →mktemp: illegal option -- -, exits nonzero. set -epropagates; the subshell substitution fails; script exits. No target is generated.- If a user works around line 52 (e.g.
gmktemp), line 58 fails next: BSDsed -i "1i // BoringSSL commit: ..." file.rstreats"1i // BoringSSL..."as the backup-suffix argument to-i, then tries to interpretfile.rsas the sed script →sed: 1: "...wrapper_x86_64-unknown-l ...": invalid command code w(or similar).
How to fix
Minimal: add one line to the header comment (after line 4), e.g. # Host: Linux (uses GNU mktemp --suffix and GNU sed -i).
Portable (if macOS support is wanted):
- Line 52:
wrap_c="$(mktemp)"; mv "$wrap_c" "$wrap_c.c"; wrap_c="$wrap_c.c"— or just drop the suffix (bindgen's--wrap-static-fns-pathdoesn't require.c). - Line 58:
{ printf '%s\n' "$STAMP"; cat "$OUT_DIR/wrapper_$triple.rs"; } > "$OUT_DIR/wrapper_$triple.rs.tmp" && mv "$OUT_DIR/wrapper_$triple.rs.tmp" "$OUT_DIR/wrapper_$triple.rs"
Either is fine; the header comment is the smallest change consistent with the script's evident Linux-cross-generation design.
Connect errors: the core now closes a failed direct-connect socket before notifying the consumer (the old shim layer's contract; two consumers had regressed to never closing, leaving the dead socket armed in level-triggered epoll - a failed redis:// or ws:// connect leaked the fd and spun the event loop at 100% CPU). Consumer closes remain idempotent no-ops; postgres/mysql regain close-before-notify ordering. Regression tests assert bounded CPU and no fd growth across repeated failed connects. Aliasing soundness: completed the raw-place conversion past the crate boundary - FilePoll registration/arm/activate, keep-alive, jsc loop accounting, spawn pid-fd watch, and dns machport registration no longer form whole-struct &mut Loop while foreign threads fetch_add pending_wakeups; the &mut accounting methods are deleted so the pattern cannot recur. HTTPThread's cross-thread queues, ThreadSafeStreamBuffer, and napi threadsafe-function entry points converted off cross-thread &mut. Worker shutdown gained rundown protection (terminating flag + external-post counter drained before VM dealloc) closing the wakeup-after-free race; TSFN calls after VM death now return napi_closing instead of silently leaking with napi_ok. close_all walks: the group walk restarts from the head instead of probing a cached pointer a GC-finalized listener can free mid-walk; the graceful socket walk snapshots the list under the tick-depth bracket (single-pass - was quadratic in TLS-deferred closes, 2x shutdown time at 8k idle TLS conns) and revalidates before each dispatch, fixing a graceful-to-RST downgrade under re-entrant adoption; the connecting walk restarts from head and the force drain now covers connecting sockets. Build: linker orderfile is invalidated when its symbol set no longer matches (the renamed socket core made inherited orderfiles silently no-op, costing +7.7MB resident text - the entire RSS delta vs main); dead C-side fault injection define removed. Cleanup: crate-wide dead_code allow removed with ~25 dead items deleted (including an unreachable depth-0 TLS entry point); duplicated helpers consolidated (twin retirement, IPC decode loop, would-block/errno module, TLS side-channel guard); test hardening (in-process servers instead of live DNS, awaited conditions, success counters, exact crash-message assertions).
| /// loop exists; null before that. Cross-thread `wakeup()` Acquire-loads it, | ||
| /// which also publishes the loop's initialization. | ||
| static UWS_LOOP: AtomicPtr<uws::Loop> = AtomicPtr::new(core::ptr::null_mut()); |
There was a problem hiding this comment.
🟡 The comment at init_once (HTTPThread.rs:1210-1212, outside the diff) still says "can fill in loop_/uws_loop/contexts" and "no other thread reads HTTP_THREAD until has_awoken is set in on_start", but this PR removes both the has_awoken: AtomicBool and uws_loop: *mut uws::Loop struct fields (moved to the UWS_LOOP AtomicPtr / HTTP_THREAD_INIT module statics) — grep confirms line 1212 is the sole remaining has_awoken reference in src/. The parallel HttpThread::new() doc comment at lines 167-169 was updated ("loop_ (and the UWS_LOOP static)"), so this is a missed sibling. Same stale-comment class as #4/#8/#13 and still-open #16/#17, but a distinct pattern (removed struct fields from the field-to-static refactor, not deleted files/crates) their grep patterns wouldn't catch. No runtime effect; drop uws_loop from the fill-in list and reword the SAFETY line to reference HTTP_THREAD_INIT. (Filed at the field-removal hunk because line 1212 has no commentable diff range.)
Extended reasoning...
What the issue is
The comment at src/http/HTTPThread.rs:1207-1212, inside init_once (outside any diff hunk), reads:
fn init_once(opts: &InitOpts) {
// Initialize the global (with timer
// started on the calling thread) BEFORE spawning, so `on_start`'s
// `crate::http_thread_mut()` finds `Some(..)` and can fill in
// `loop_`/`uws_loop`/contexts.
// SAFETY: `init_once` runs under `Once`; no other thread reads
// `HTTP_THREAD` until `has_awoken` is set in `on_start`.
unsafe {
(*crate::HTTP_THREAD.get()).write(HttpThread::new());
}
crate::HTTP_THREAD_INIT.store(true, core::sync::atomic::Ordering::Release);Both uws_loop (as an HttpThread field) and has_awoken are removed by this PR. The diff shows:
- pub uws_loop: *mut uws::Loop,— replaced by the module-levelstatic UWS_LOOP: AtomicPtr<uws::Loop>at line 99.- pub has_awoken: AtomicBool,— replaced by theHTTP_THREAD_INITgate (the.store(true, Release)on line 1216, four lines below the stale comment).- // Release: publishes uws_loop/loop_ to cross-thread wakeup() readers (which Acquire-load has_awoken).and- thread.has_awoken.store(true, Ordering::Release);— the mechanism the SAFETY comment describes was deleted fromon_start.
rg has_awoken src/ returns exactly one hit — line 1212 itself — confirming the identifier no longer exists anywhere else.
Why nothing prevents it
It is a // comment — nothing enforces it. The diff did update the parallel comment at HttpThread::new() (lines 167-169 now correctly read "loop_ (and the UWS_LOOP static) are filled in by on_start"), so the author was aware of the class; this second instance ~1000 lines below was missed in the same pass.
Relationship to previous comments
This PR has already accepted and fixed ~7 rounds of the same "development-history narration in comments" class: #4 (lib.rs/cabi.rs), #8 (event_loop/README.md), #13 (lib.rs:40), and the still-open #16 (deleted C filenames) / #17 (deleted Rust crate names). This is a distinct instance: it references removed struct fields introduced by this PR's field-to-static refactor (per the diff, the HttpThread struct dropped uws_loop/has_awoken and the state moved to module statics UWS_LOOP/HTTP_THREAD_INIT), not deleted files or crates — so the grep patterns from #16 (epoll_kqueue|bsd\.c|SocketKind) and #17 (bun_uws\b|bun_uws_sys|uws_sys/SocketKind) would not match has_awoken or uws_loop. Not a duplicate.
Impact
No runtime effect — pure comment staleness, hence nit. The concrete downside: this is a SAFETY comment justifying an unsafe block, and it names the wrong synchronization primitive. A reader auditing the unsafe { (*crate::HTTP_THREAD.get()).write(...) } for data races would look for has_awoken in on_start (which no longer exists), not HTTP_THREAD_INIT (the actual gate on line 1216, right below). The actual invariant is still sound — only the description is wrong. Per CLAUDE.md "Comments carry only durable non-obvious content: invariants, ownership/lifetime contracts, SAFETY justifications" — a SAFETY comment that names a nonexistent field is exactly the kind of drift that rule targets.
Step-by-step proof
sed -n '1210,1212p' src/http/HTTPThread.rsat HEAD ee37e3a:// `loop_`/`uws_loop`/contexts. // SAFETY: `init_once` runs under `Once`; no other thread reads // `HTTP_THREAD` until `has_awoken` is set in `on_start`.rg has_awoken src/→ single hit:src/http/HTTPThread.rs:1212. The field is gone from all code.- PR diff at
HTTPThread.rs:- pub uws_loop: *mut uws::Loop,and- pub has_awoken: AtomicBool,removed from theHttpThreadstruct;+static UWS_LOOP: AtomicPtr<uws::Loop>added at line 99;- thread.has_awoken.store(true, Ordering::Release);removed fromon_start. sed -n '167,169p' src/http/HTTPThread.rs→ the parallelHttpThread::new()doc comment was updated in this PR: "loop_(and theUWS_LOOPstatic) are filled in byon_start". So the author touched one of the two comments describing this state; line 1210-1212 is the missed sibling.- Line 1216 (
crate::HTTP_THREAD_INIT.store(true, Release)) is the actual publish gate the SAFETY comment should name. - The still-open Fix
browsermap #16/Fix JSX parser bug:// commentafter tagName and before closing tag is broken #17 grep patterns match neitherhas_awokennoruws_loop, so this is not covered by them.
How to fix
// Initialize the global (with timer started on the calling thread) BEFORE
// spawning, so `on_start`'s `crate::http_thread_mut()` finds `Some(..)`
// and can fill in `loop_` (and the `UWS_LOOP` static)/contexts.
// SAFETY: `init_once` runs under `Once`; no other thread reads
// `HTTP_THREAD` until `HTTP_THREAD_INIT` is Release-stored below.— i.e., mirror the already-updated HttpThread::new() comment for the fill-in list, and point the SAFETY line at HTTP_THREAD_INIT (the store on line 1216) instead of the removed has_awoken.
(Filed at the field-removal hunk because line 1212 has no commentable diff range.)
| "src/shell_parser", | ||
| "src/tcc_sys", | ||
| "src/uws_sys", | ||
| "src/usockets", |
There was a problem hiding this comment.
🟡 🟡 Companion to still-open #19 (src/bssl/bindings/README.md → .rewrite-specs/tls-semantics.md): three more gitignored working-notes references at src/usockets/Cargo.toml — line 26 (protocol.rs; safe-protocol.md), line 28 (consumers/04), line 31 (tls-semantics.md Part 2d). None exist in-tree (src/usockets/docs/ has only cabi.md/design.md/semantics.md/tls.md; .rewrite-specs/ is gitignored by this PR at .gitignore:219), and two of the three (safe-protocol.md, consumers/04) are NOT caught by #19's suggested rewrite-specs|tls-semantics grep. Same class as #4/#8/#13/#16/#17/#19/#21; no runtime effect. Fix: drop the three parentheticals — the sentences read fine without them. (Filed against workspace Cargo.toml because the new src/usockets/Cargo.toml file has no commentable diff line ranges.)
Extended reasoning...
What the issue is
src/usockets/Cargo.toml is a new file added by this PR. Three of its dependency comments cite working-notes files that do not exist in the committed tree:
- Line 26:
# Protocol v2 owner refcounting (protocol.rs; safe-protocol.md).—safe-protocol.mddoes not exist.src/usockets/docs/contains onlycabi.md,design.md,semantics.md,tls.md(verified vials). - Line 28:
# Platform-splitaddrinfotype shared with the DNS bridge (consumers/04).— noconsumers/directory exists anywhere insrc/usockets/or the repo root. - Line 31:
# Raw BoringSSL bindings (tls-semantics.md Part 2d): re-export of the vendored—tls-semantics.mddoes not exist in-tree; it lives in.rewrite-specs/, which this same PR adds to.gitignoreat line 219.
All three are references into the author's local .rewrite-specs/ working-notes directory, which the PR itself gitignores (below the existing /specs/ entry commented "Kept locally for the ongoing work; not part of the source tree"). No reader — reviewer or future maintainer — will ever have those files.
Relationship to still-open comment #19
Comment #19 (still unresolved, filed at src/bssl/bindings/README.md:6) covers the same "gitignored working notes" pattern class at a different file — src/bssl/bindings/README.md lines 6 and 41, both citing tls-semantics.md. This finding is a companion at a distinct location (src/usockets/Cargo.toml), and critically: two of the three references here would NOT be caught by #19's suggested grep pattern. #19 recommends rg 'rewrite-specs|tls-semantics' src/ packages/; that catches line 31 but misses safe-protocol.md (line 26) and consumers/04 (line 28). So a fix pass addressing #19 would leave lines 26 and 28 stale.
Verified: rg 'safe-protocol|consumers/0[0-9]|tls-semantics' src/ packages/ hits only src/usockets/Cargo.toml (3 lines) and src/bssl/bindings/README.md (2 lines, covered by #19).
Why nothing prevents it
These are # TOML comments — nothing enforces them. This is the same "development-history narration in committed docs" class the PR has already accepted and fixed ~7 rounds of (#4 lib.rs/cabi.rs, #8 event_loop/README.md, #13 lib.rs:40, #16 deleted C filenames, #17 deleted Rust crate names, #19 gitignored .rewrite-specs, #21 removed struct fields), all of which the author has fixed or acknowledged. The bar this PR's review has established is clear: dangling references to nonexistent/gitignored files in committed docs are worth flagging as nits.
Impact
No runtime effect — pure comment staleness, hence nit. The concrete downside: a reader trying to understand why bun_ptr / bun_dns / bun_bssl are dependencies of the new socket crate is pointed at three files they cannot find. Per CLAUDE.md "Comments carry only durable non-obvious content: … no bug history — that belongs in the PR description". The crate's own docs/ directory (design.md, cabi.md) already carries the durable references; the parentheticals add nothing a checked-out reader can follow.
Step-by-step proof
sed -n '26p' src/usockets/Cargo.toml→# Protocol v2 owner refcounting (protocol.rs; safe-protocol.md).ls src/usockets/docs/→cabi.md design.md semantics.md tls.md. Nosafe-protocol.mdanywhere in the crate or repo.sed -n '28p' src/usockets/Cargo.toml→# Platform-splitaddrinfotype shared with the DNS bridge (consumers/04).ls src/usockets/consumers/→ No such file or directory.find . -name '04*' -path '*consumers*'→ nothing.sed -n '31p' src/usockets/Cargo.toml→# Raw BoringSSL bindings (tls-semantics.md Part 2d): re-export of the vendoredls .rewrite-specs/→ No such file or directory. This PR's.gitignore:219adds.rewrite-specs/.- test #19's grep pattern
rewrite-specs|tls-semanticsmatches line 31 but NOT lines 26 or 28 — so this is not a duplicate; fixing test #19 alone leaves two of the three references stale.
How to fix
Drop the three parentheticals — the sentences read fine without them:
- Line 26:
# Protocol v2 owner refcounting (protocol.rs). - Line 28:
# Platform-splitaddrinfotype shared with the DNS bridge. - Line 31:
# Raw BoringSSL bindings: re-export of the vendored
The protocol.rs reference on line 26 is fine to keep (that file exists at src/usockets/protocol.rs); only the ; safe-protocol.md suffix is dead.
(Filed against workspace Cargo.toml:90 because the new src/usockets/Cargo.toml file has no commentable diff line ranges — same convention every other new-file finding on this PR has used.)
|
Triage of the new asan-lane SIGABRT (test-worker-message-port-transfer-terminate.js): pre-existing, not from this branch. A/B on the CI asan artifacts under identical load: this head 117/320 aborts vs the previous head 103/320 — same rate, identical assertion ( The dead-code-escapes failure was an inventory update for two justified platform-split allows — fixed in b061f12. |
| // `JsCell` and callers do not hold the borrow across JS re-entry that | ||
| // touches `ipc_data` itself. | ||
| unsafe { self.ipc_data.get_mut() }.as_mut() | ||
| // SAFETY: single JS-mutator thread; the heap `IpcData` outlives this |
There was a problem hiding this comment.
we dont need safety comments if we are not using unsafe
| // so `&mut Store` is materialised only *after* `&mut self` is retired | ||
| // inside `deinit_possibly_defer` (via `file_polls_mut()`). | ||
| let loop_ = vm.loop_mut(); | ||
| let loop_ = vm.loop_(); |
There was a problem hiding this comment.
loop_ is not a good name IMO
|
@robobun rebase |
|
Tried to bring this up to date with main; stopping to flag scope before touching anything. Since the last sync ( ~30 are modify/delete on files this PR deletes. Main landed 34 commits that modify the C usockets core this PR removes (
Some of these the rewrite may already cover by construction (the PR body calls out generation-checked handles and core-owned close ordering, which plausibly subsumes #36467 and #37661), but deciding that per-fix and porting the rest is design work on the crate itself, not something I should do by picking a side of a conflict marker. The other ~95 are content conflicts in consumer files ( I have not pushed anything. @Jarred-Sumner this is effectively a month of upstream usockets work to re-port into the crate; seems like your call how to sequence it. Happy to take the mechanical consumer-file conflicts once the deleted-file fixes have been ported, or to do a merge that takes HEAD for the deleted files and leaves the port list above as a checklist, if that is the preferred workflow. Say which. |
4 participants
What
Rewrites the uSockets C core as a native Rust crate,
bun_usockets(src/usockets/), and deletes the C. Everything socket-shaped in the runtime now runs on it:Bun.serve(HTTP + WebSocket via uWS),fetchand the HTTP client thread,Bun.listen/Bun.connect,node:net/node:tls/node:http, the WebSocket client, Postgres/MySQL/Valkey,Bun.spawnIPC,Bun.udpSocket/node:dgram, and DNS-backed connects.Deleted:
packages/bun-usockets/src/{bsd,loop,socket,context,udp,fault_inject}.c,eventing/,crypto/openssl.c,crypto/sni_tree.cpp(~11.4k lines of C), plus thebun_uws_sys/bun_uwslayout-mirror crates (~9.5k lines). Survivors:quic.c(lsquic glue), the root-cert data tables, and the uWS C++ layer.Design
Memory model: generational slabs, no relocation, reclaimable high-water
Each loop owns chunked slabs of socket slots (256 slots per mmap-reserved chunk; separate size classes so uWS kinds keep their ext data contiguous with the header, exactly like the C's single-allocation layout). Slot addresses are stable for the loop's lifetime. Every externally stored socket reference is a 16-byte
SocketRef { ptr, generation }; each operation validates the generation with one load+compare, and a stale handle behaves like today's detached socket — every method is a safe no-op. Use-after-free through a socket handle is impossible by construction, not by review.Reclamation: when a chunk empties, its pages are decommitted with
MADV_DONTNEED(MEM_RESETon Windows) — RSS returns to the OS while the address range stays mapped, so a stale handle's validation read faults in a zero page (generation 0 never matches). A per-chunk epoch, stored outside the decommittable memory and packed into the u64 generation's high bits, makes handles from before a decommit permanently invalid, killing the ABA reuse case. Hysteresis keeps the most-recently-emptied chunk committed so churn at a chunk boundary doesn't thrash. Loops fully release their reservations at teardown, so worker churn doesn't accumulate address space.Close is a two-phase protocol (unchanged from the C, now type-enforced): close unlinks onto the loop's closed list immediately; the generation bumps and the slot returns to the free list only in the tick postlude. That deferred window is what makes re-entrant closes from user JS callbacks survivable, and it's now the only death path.
Dispatch: const tables, zero dynamic dispatch, zero lazy init
Event dispatch is: kernel udata → generation check → one indirect call through a per-kind vtable in
.rodata. The kind→vtable and kind→owner-ops tables are fully const-initialized statics assembled in the one crate that sees every protocol type, reached by the core through a link-timeextern "Rust" safe static— no registration calls, noOnceLock, nodynanywhere in the crate. A new socket kind that isn't wired into the table is a compile error (array shape), not a first-dispatch panic. The generation check also closes a real C bug: a stale kernel event for a recycled poll slot now resolves to a dead slot and is dropped (the C had a narrow stale-udata window after zero-event resize).Safe consumer interface
Consumers implement a
Protocoltrait whose handlers receive&Owner(interior-mutable) plus the generation-checked handle. The dispatch trampoline — written once, inside the unsafe jail — takes a strong ref on the owner before invoking any handler and drops it after, so re-entrant JS can no longer free a connection object out from under its own callback. That guarantee deletes the ref-guard/raw-pointer discipline every consumer used to hand-roll: socket-lifecycleunsafein the migrated consumers (HTTP client, SQL drivers, Valkey, WS client, IPC, UDP,Bun.listen/connect) is now zero. The core also owns terminal ref release exactly-once on every path, including silent SEMI_SOCKET closes that previously required per-consumer compensation code.All unsafe in the crate is jailed in
src/usockets/unsafe_core/(slab, syscall edges, FFI, trampolines); the rest — loop tick, TLS state machine, timeouts, UDP, dispatch policy — is compiler-enforced safe (deny(unsafe_code)). Outside the jail there are two one-line unsafe blocks.One event loop abstraction
File polls (fd watchers, process reaping via
EVFILT_PROC, machport wakeups, memory-pressure) previously formed a parallel poll universe with rawepoll_ctl/keventcalls, a tagged-pointer udata convention, and a dispatch back-channel through the loop'sready_pollsarray. They now register through the same generational registry as sockets, with the same owner-held-across-callback guarantee. Noepoll_ctl/keventexists outsidesrc/usockets/backend/(per-thread queues like the watcher and the IO request loop keep their own kernel queues by design).TLS
The TLS layer targets the official vendored
bssl-sysbindings instead of hand-written externs. Ciphertext staging is loop-shared — one batch buffer and one spill slot per loop, O(1) memory regardless of connection count, same 16KB-record/128KB-flush thresholds as the C — with the two things that made the C version fragile fixed in the types: the buffer owner is a generational handle (can't dangle), and the save/restore re-entrancy protocol around JS callbacks running inside handshakes (ALPN/SNI) is an RAII scope guard. Sockets never relocate onupgradeTLS: adoption restamps the kind in place, deleting both the realloc+memcpy per WebSocket upgrade and the "callback must return the possibly-moved pointer" contract.The C++ boundary
The surviving uWS C++ layer calls Rust through a minimal
extern "C"surface (cabi.rs, ~63 functions) sized to exactly the symbols it uses. Struct-field pokes in the C++ were replaced with accessors sous_socket_t/us_loop_tare fully opaque to C++; onlyus_socket_group_tremains public repr(C) (uWS embeds it by value). Buffer args are length-checked at the boundary.Approach
The C's behavior was extracted into numbered rules before any code was written — the loop tick sequence, poll-layer semantics per backend, socket lifecycle, write path, timeout wheels, connect/happy-eyeballs, TLS handshake/shutdown ordering — and the implementation was reviewed rule-by-rule against them. Those rules ship as crate docs (
src/usockets/docs/), and code comments cite them by number. Everything perf-relevant carries over exactly:epoll_pwait2with the ENOSYS latch,kevent64changelists, sweep deadlines folded into the poll timeout (no timer fd), low-priority handshake parking (5/tick), corking, ENOBUFS/ENOMEM-as-would-block, the close-code trichotomy (close_notify / RST / fast-shutdown), half-open,sendmmsg/recvmmsgUDP batching. Known C quirks are ported verbatim and listed indocs/semantics.mdrather than silently fixed; the handful of deliberate deviations (no relocation, generation-checked stale events, core-owned SEMI_SOCKET ref release) are documented indocs/design.md.Testing
cargo checkclean on all 10 cross-targets, including the Windows winsock layer; first clean--all-targetsworkspace check.tls.connect({socket})over a live socket and worker loop teardown.On-box debug-build perf signals (proper benchmarks to follow): websocket 300k-echo 33.4s vs 35.4s pre-rewrite; the pipe-leak RSS test measures 121–159MB vs 240MB pre-rewrite.