Conversation
WalkthroughChangesWebSocket ping support
Possibly related issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/js/thirdparty/undici.js`:
- Line 3: The module eagerly requires diagnostics_channel and creates its
channels even when WebSockets are unused; move the require and channel
initialization into the WebSocket constructor and ping paths, reusing a lazy
shared initialization so both paths obtain the channels only when needed while
retaining the node:-prefixed require.
- Around line 413-416: Update the WebSocket ping logic around BunWebSocket.OPEN
to throw InvalidStateError when readyState is CONNECTING, while keeping CLOSING
and CLOSED as no-ops. Preserve the existing payload and no-payload ws.ping()
calls for the OPEN state.
- Around line 400-411: Update the receiver validation in ping to check the
Undici wrapper WebSocket brand rather than BunWebSocket, so native Bun WebSocket
instances are rejected consistently with the private handler brand check.
Preserve the existing Buffer|undefined payload validation and 125-byte limit.
In `@test/js/first_party/undici/undici.test.ts`:
- Line 212: Change the “undici WebSocket ping” suite to describe.concurrent so
its independent network tests run in parallel. Update each diagnostics-channel
subscriber used by the suite to filter messages by message.websocket before
resolving, preserving isolation from other concurrent clients.
- Around line 290-296: Add the missing argument-validation cases to the existing
“ping() validates its arguments like undici” test: cover null payloads,
Uint8Array payloads, empty-string and zero payloads, and undefined/null
websocket arguments. Assert the shim’s expected behavior for each variant,
matching the validation logic in ping and preserving the existing boundary and
invalid-type assertions.
- Around line 290-303: Update the ping() validation test to assert ws.readyState
is not OPEN immediately before the no-op assertions, ensuring the socket remains
in the not-open state when testing ping(ws, ...) and ping(ws,
Buffer.alloc(125)).
- Around line 266-288: Update the “undici:websocket:ping fires when the server
sends a ping” test so its promise resolver rejects on WebSocket error and close
events, matching the failure-handling pattern used by the first opened test.
Register these listeners before awaiting pingMessage.promise and retain the
existing cleanup and ping assertions.
🪄 Autofix
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: ddb70a85-b071-4e4c-b8d5-b86448930a54
📒 Files selected for processing (2)
src/js/thirdparty/undici.jstest/js/first_party/undici/undici.test.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate, though all three are related and already linked in the description:
This PR is the minimal self-contained fix for #37110: as long as the shim exists and answers bare |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/js/thirdparty/undici.js`:
- Around line 382-383: Capture stable references to the ArrayBuffer constructor
and its isView method during module initialization, then update the data
conversion checks in the buffer-handling path to use those captured intrinsics
instead of mutable globals. Preserve the existing byteOffset and byteLength
behavior for views.
🪄 Autofix
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: c9fdf175-49f7-4ff4-9df2-98eda3e77d04
📒 Files selected for processing (2)
src/js/thirdparty/undici.jstest/js/first_party/undici/undici.test.ts
There was a problem hiding this comment.
Both earlier nits are addressed and I didn't find anything new, but deferring to a maintainer: the exported WebSocket changes from the native class to a subclass (previously require('undici').WebSocket === globalThis.WebSocket), and with #34635 / #36102 / #30561 all open against the same surface, the sequencing — and whether landing this alongside #34635 would double-publish on the ping/pong channels — is a call for a human.
What was reviewed:
ping()argument validation and OPEN-gated send against undici 7.x semantics;controlFramePayloadcoercion for the non-nodebufferbinaryTypepath.- Tests: failure events wired to reject, per-socket channel filtering under
describe.concurrent,readyStateprecondition asserted before the no-op path. - Checked that
$isTypedArrayView+ capturedNativeArrayBuffermatch the tamper-resistance pattern inws.js.
Extended reasoning...
Overview
The PR extends src/js/thirdparty/undici.js (~50 lines) and adds ~140 lines of tests to test/js/first_party/undici/undici.test.ts. It exports a module-level ping(ws, payload) and replaces the exported WebSocket with a subclass of the native class whose constructor attaches ping/pong event listeners that republish to the undici:websocket:ping/undici:websocket:pong diagnostics channels. No native code is touched. All prior review feedback on this PR — my two nits (Buffer coercion for non-default binaryType, wiring failure events to reject in the ping test), CodeRabbit's items (readyState precondition, error/close rejection, extra validation variants, captured ArrayBuffer intrinsics), and the comment-cop JSDoc note — has been applied and the threads are resolved.
Security risks
None identified. This is a JS-only compat shim; ping() validates its inputs (WebSocket instance check, Buffer-only payload, ≤125 bytes) before delegating to the native ws.ping(), and the diagnostics-channel publish is guarded by hasSubscribers. No auth, crypto, filesystem, or network-origin decisions are involved.
Level of scrutiny
Medium. The runtime change is small and well-tested (fails on main, passes on the PR under both debug/ASAN and release), but it does alter user-visible API surface: require('undici').WebSocket was previously the native WebSocket class itself and is now a subclass. instanceof globalThis.WebSocket still holds and the motivating consumer (@slack/socket-mode) works, but strict identity checks against globalThis.WebSocket would now differ, which is the kind of compat-surface tweak a maintainer should sign off on.
Other factors
Three open PRs overlap this surface: #34635 publishes these same channels natively in WebSocket.cpp, #36102 resolves bare undici to the installed package, and #30561 removes the shim entirely. The author's rationale for this PR standing alone is reasonable, but the description's claim that #34635 "supersedes" the subclass listeners isn't automatic — if both land as-is, each ping/pong would publish twice (once natively, once from the subclass listener). That coordination, plus the broader question of which direction to take the undici shim, is a maintainer decision rather than something to auto-approve.
|
On the coordination point: agreed that the supersede is not automatic. If #34635 lands after this, its change should drop the two subclass listeners here (they are self-contained in the WebSocket constructor in src/js/thirdparty/undici.js); if this lands after #34635, I will remove them from this diff before merge. The ping() export is orthogonal and needed either way. On identity: require('undici').WebSocket is now a subclass rather than globalThis.WebSocket itself, so instanceof globalThis.WebSocket still holds but strict equality with the global does not. Node has the same non-identity in reverse for code that patches one and expects the other; happy to adjust if a maintainer prefers attaching listeners differently. |
|
@robobun any plans to merge this? 👀 |
99db264 to
07d6a6c
Compare
|
Updated 1:02 PM PT - Aug 21st, 2026
✅ @robobun, your commit 139022da2b16fd437150fc4b1324729cdb83064a passed in 🧪 To try this PR locally: bunx bun-pr 37111That installs a local version of the PR into your bun-37111 --bun |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs; my two earlier inline nits (Buffer coercion for non-default binaryType, and wiring failure events in the ping test) have both been addressed. Deferring the approval to a maintainer because the exported WebSocket is now a subclass rather than the native class itself — the author flagged that identity change for maintainer input — and because this overlaps with #34635 / #36102 / #30561, which need a human to sequence.
What was reviewed:
ping()argument validation and no-op-while-not-OPEN semantics against undici 7.x behavior.controlFramePayloadcoercion path fornodebuffer/arraybuffer/typed-array inputs; capturedArrayBuffer+$isTypedArrayViewfor tamper-resistance.- Test hermeticity: per-socket channel filtering under
describe.concurrent, error/close wired to reject the awaited resolvers.
Extended reasoning...
Overview
The PR adds two things to the built-in undici shim (src/js/thirdparty/undici.js): a module-level ping(ws, payload) export mirroring undici v7's, and a WebSocket subclass of the native class that republishes Bun's native ping/pong events onto the undici:websocket:ping/undici:websocket:pong diagnostics channels. ~50 lines of shim JS plus ~140 lines of tests in test/js/first_party/undici/undici.test.ts. No native code touched.
Security risks
None identified. This is a thin JS shim over existing native WebSocket behavior; there is no auth, crypto, filesystem, or untrusted parsing surface. The 125-byte payload cap is validated before delegating to native ws.ping().
Level of scrutiny
Moderate. The logic is small and well-tested, but it changes user-facing API surface: require('undici').WebSocket was previously identical to the native WebSocket constructor and is now a subclass (so === globalThis.WebSocket becomes false, and every undici-constructed socket now carries two extra event listeners). The author explicitly flagged this identity change for maintainer input in the thread. That is the kind of API-shape call a human should sign off on rather than a bot.
Other factors
Three open PRs touch the same surface with different strategies — #34635 publishes these channels natively in WebSocket.cpp, #36102 resolves bare undici to the installed package, and #30561 deletes the shim outright. If #34635 lands alongside this, the channels would double-publish until one side is removed; the author acknowledged the coordination burden. All prior CodeRabbit and claude[bot] inline comments are resolved, tests pass on both debug/ASAN and release per the PR evidence, and a community user is asking for merge — so the remaining gate is a maintainer decision on the subclass approach and PR sequencing, not code correctness.
…ics channels
The builtin undici shim did not export ping() and its WebSocket never
published the undici:websocket:ping/pong diagnostics channels, so
@slack/socket-mode heartbeats threw (ping is not a function) and its
health monitor counted every pong as missed, producing a permanent
reconnect loop.
ping(ws, payload) mirrors undici's validation (Buffer-or-undefined
payload, 125-byte cap, no-op unless open) and sends a ping frame via
the native WebSocket. The shim's WebSocket is now a subclass that
publishes {payload, websocket} to the ping/pong channels when frames
arrive, matching undici's message shape.
Fixes #37110
Run the suite concurrently, filter diagnostics-channel messages by websocket identity, reject on error/close instead of hanging, pin the CONNECTING precondition for the no-op case, and cover null/Uint8Array payloads and undefined/null receivers.
07d6a6c to
139022d
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs — my earlier inline comments (Buffer coercion for non-default binaryType, wiring failure events to reject) have both been addressed. A maintainer look would still be worthwhile for the two design points the author flagged: the exported WebSocket is now a subclass (so require('undici').WebSocket !== globalThis.WebSocket), and this overlaps with #34635/#36102/#30561 which take different approaches to the same surface.
What was reviewed:
ping()argument validation and OPEN-state gating against undici 7.x semantics — matches (Buffer-only payload, 125-byte cap, no-op when not OPEN).controlFramePayloadcoercion — capturedArrayBuffer+$isTypedArrayViewintrinsic, covered by thebinaryType='arraybuffer'test.- Test hygiene —
describe.concurrentwith per-socket channel filtering, error/close wired to reject,readyStateasserted before the no-op path.
Extended reasoning...
Overview
The PR adds two things to Bun's built-in undici shim (src/js/thirdparty/undici.js): a module-level ping(ws, payload) export matching undici v7's, and a WebSocket subclass whose instances publish { payload, websocket } on the undici:websocket:ping/undici:websocket:pong diagnostics channels. It fixes #37110 (@slack/socket-mode reconnect loop). ~50 lines of shim JS plus ~140 lines of new tests in test/js/first_party/undici/undici.test.ts. No native changes.
Security risks
None identified. Shim-only JS, no new parsing of untrusted input beyond the existing native WebSocket.ping() path (which already enforces the 125-byte cap independently). The tamper-resistance nit (mutable ArrayBuffer global) was addressed with a module-captured reference and the $isTypedArrayView intrinsic.
Level of scrutiny
Medium. The code is small and additive, tests pass on debug+ASAN and release, and all prior automated feedback (CodeRabbit and my own inline comments) has been addressed and marked resolved. What keeps this from being a rubber-stamp is that it makes a user-visible API-shape decision — the exported WebSocket becomes a subclass of the native one rather than the native class itself — and the author explicitly said they're "happy to adjust if a maintainer prefers attaching listeners differently."
Other factors
Three open PRs address the same failure class differently: #34635 publishes the websocket diagnostics channels natively in WebSocket.cpp, #36102 resolves bare undici to the installed npm package with the shim as fallback, and #30561 removes the shim entirely. The author's position (this is the minimal self-contained fix that stands alone; ping() is needed regardless; the subclass listeners are trivially removable if #34635 lands) is reasonable, but choosing which of these lands and in what order is a maintainer call. The author's own comment on 2026-08-07 explicitly invites that input on both the coordination and the subclass-vs-alternative question, so deferring rather than approving.
|
A second report of this came in (Bolt 5 / The script below drives
repro.mjs// bun add @slack/socket-mode@3.0.1 undici@7 ws
import { createRequire } from "node:module";
import { EventEmitter } from "node:events";
const require = createRequire(import.meta.url);
console.log("undici.ping is", typeof require("undici").ping);
const { SlackWebSocket } = require("@slack/socket-mode/dist/src/SlackWebSocket.js");
const server = globalThis.Bun
? Bun.serve({ port: 0, fetch: (req, srv) => (srv.upgrade(req) ? undefined : new Response("no", { status: 400 })), websocket: { message() {} } })
: await new Promise(async (resolve) => { const { WebSocketServer } = await import("ws"); const wss = new WebSocketServer({ port: 0 }, () => resolve({ port: wss.address().port })); });
const log = (lvl) => (...m) => console.log(lvl, ...m);
const logger = { debug: log("debug"), info: log("info"), warn: log("warn"), error: log("error"), setLevel() {}, getLevel: () => "debug", setName() {} };
const client = new EventEmitter();
client.on("close", () => console.log(">>> CLOSED (the reconnect churn)"));
const ws = new SlackWebSocket({ url: `ws://127.0.0.1:${server.port}/`, client, logger, logLevel: "debug", pingPongLoggingEnabled: true, clientPingTimeoutMS: 900, serverPingTimeoutMS: 30000 });
await ws.connect();
setTimeout(() => process.exit(0), 5000); |
steipete
left a comment
steipete
left a comment
There was a problem hiding this comment.
Confirmed against OpenClaw's real Slack provider interop lane. Shipping Bun 1.4.2 fails with ping is not a function; applying this PR's final implementation to current Bun main makes the complete 19-test Slack provider lifecycle file pass under an ASan debug build. The complete Bun undici test file also passes (26/26).
AI-assisted validation: Codex was used to trace the OpenClaw failure, inspect the Bun and Undici contracts, build Bun, and run the tests; I reviewed the implementation and results.
|
Thanks for the independent validation against a real Slack Socket Mode lifecycle. That covers the end-to-end path the unit tests cannot reach (an actual Slack peer answering client pings), so it is useful evidence for the reviewer. |
Ports oven-sh#37111 into the OpenClaw compatibility build.
Ports oven-sh#37111 into the OpenClaw compatibility build. (cherry picked from commit 278105f)
Ports oven-sh#37111 into the OpenClaw compatibility build. (cherry picked from commit 278105f)
This branch has not been deployed
3 participants
Fixes #37110.
What does this PR do?
Bun aliases the bare
undicispecifier to its builtin shim, and the shim covered neither the module-levelping()export nor theundici:websocket:ping/undici:websocket:pongdiagnostics channels that undici v7 publishes.@slack/socket-mode@3.x(the transport under Bolt's SocketModeReceiver) uses both, so every connection died on its first heartbeat withTypeError: ping is not a functionand the client reconnected forever; even with that swallowed, its health monitor would count 3 missed pongs (the pong channel never fires) and disconnect anyway.How
Shim-level changes in
src/js/thirdparty/undici.js, no native changes:ping(ws, payload)mirrors undici's semantics: payload must be aBufferorundefinedand at most 125 bytes (TypeErrorotherwise, with undici's messages), and the frame is only sent while the socket is open (no-op otherwise). It delegates to the nativeWebSocket's nonstandardping()method, the same one thewsshim already relies on.WebSocketis now a subclass of the native class that listens for the nativeping/pongevents and publishes{ payload, websocket }(payload is aBuffer) to theundici:websocket:ping/undici:websocket:pongchannels when they have subscribers, matching undici's message shape.@slack/socket-modeconstructs its socket vianew (require("undici").WebSocket)(...)and identity-checksmessage.websocket, so the subclass covers it.Related: #36102 would resolve bare
undicito the installed package when present (which also fixes this scenario when the real package is installed), and #34635 publishes the websocket channels natively for all WebSocket instances. This PR keeps the shim itself correct for this surface and stands alone; if #34635 lands, its native publish supersedes the subclass listeners here.Verification
New tests in
test/js/first_party/undici/undici.test.ts:pingis exported as a function (fails on current bun withSyntaxError: Export named 'ping' not found in module 'undici')ping(ws, payload)delivers a ping frame to aBun.servewebsocket server, and the server's pong reply publishes onundici:websocket:pongwith the echoedBufferpayload and the originating websocketundici:websocket:pingTypeError, payloads over 125 bytes throw, and pinging a not-yet-open socket is a no-opbun bd test test/js/first_party/undici/undici.test.ts: 14 pass, 0 fail. The pre-existingundici-primordials.test.tsalso passes.[review] gate passed · iteration 2 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 1 rejected · iteration 2
evidence per changed file