diff --git a/.changeset/socket-mode-pingpong-instanceof.md b/.changeset/socket-mode-pingpong-instanceof.md new file mode 100644 index 000000000..4d68185c3 --- /dev/null +++ b/.changeset/socket-mode-pingpong-instanceof.md @@ -0,0 +1,5 @@ +--- +"@slack/socket-mode": patch +--- + +fix(socket-mode): stop spurious ping/pong WARN from other undici WebSockets ([#2743](https://github.com/slackapi/node-slack-sdk/issues/2743)). Route diagnostics frames by reference identity instead of `instanceof`, which failed across undici copies (e.g. Node's global `WebSocket`). diff --git a/packages/socket-mode/src/SlackWebSocket.test.ts b/packages/socket-mode/src/SlackWebSocket.test.ts index 3ed500919..a1fde4e4f 100644 --- a/packages/socket-mode/src/SlackWebSocket.test.ts +++ b/packages/socket-mode/src/SlackWebSocket.test.ts @@ -1,4 +1,5 @@ import assert from 'node:assert/strict'; +import { channel } from 'node:diagnostics_channel'; import { afterEach, beforeEach, describe, it } from 'node:test'; import { ConsoleLogger } from '@slack/logger'; import EventEmitter from 'eventemitter3'; @@ -247,4 +248,75 @@ describe('SlackWebSocket', () => { assert.strictEqual((sws as unknown as { defaultSocket: unknown }).defaultSocket, null); }); }); + + describe('ping/pong diagnostics channel filtering', () => { + const pingChannel = channel('undici:websocket:ping'); + const pongChannel = channel('undici:websocket:pong'); + + function connect() { + const ws = new WSMock(); + const SWS = proxyquire.load('./SlackWebSocket', { + undici: { + WebSocket: class Fake { + constructor() { + // biome-ignore lint/correctness/noConstructorReturn: for test mocking purposes + return ws; + } + }, + CloseEvent, + ErrorEvent, + MessageEvent, + ping: () => {}, + }, + }).SlackWebSocket; + const logger = new ConsoleLogger(); + const warn = sandbox.spy(logger, 'warn'); + const sws = new SWS({ + url: 'ws://127.0.0.1/', + client: new EventEmitter(), + clientPingTimeoutMS: 1, + serverPingTimeoutMS: 1, + logger, + }); + const monitorPingFromSlack = sandbox.stub( + sws as unknown as { monitorPingFromSlack: () => void }, + 'monitorPingFromSlack', + ); + const lastPong = () => (sws as unknown as { lastPongReceivedTimestamp?: number }).lastPongReceivedTimestamp; + sws.connect(); + return { ws, warn, monitorPingFromSlack, lastPong }; + } + + // A plain object stands in for a WebSocket from a different undici copy: it fails `instanceof` our + // import, which is exactly the frame that used to warn. A WSMock instance would pass `instanceof`. + const foreignSocket = {}; + + it('ignores a ping for another undici copy socket, without warning', () => { + const { warn, monitorPingFromSlack } = connect(); + pingChannel.publish({ websocket: foreignSocket, payload: Buffer.from('x') }); + sinon.assert.notCalled(monitorPingFromSlack); + sinon.assert.notCalled(warn); + }); + + it('processes a ping for this socket', () => { + const { ws, warn, monitorPingFromSlack } = connect(); + pingChannel.publish({ websocket: ws, payload: Buffer.from('x') }); + sinon.assert.calledOnce(monitorPingFromSlack); + sinon.assert.notCalled(warn); + }); + + it('ignores a pong for another undici copy socket, without warning', () => { + const { warn, lastPong } = connect(); + pongChannel.publish({ websocket: foreignSocket, payload: Buffer.from('x') }); + assert.strictEqual(lastPong(), undefined); + sinon.assert.notCalled(warn); + }); + + it('processes a pong for this socket', () => { + const { ws, warn, lastPong } = connect(); + pongChannel.publish({ websocket: ws, payload: Buffer.from('x') }); + assert.strictEqual(typeof lastPong(), 'number'); + sinon.assert.notCalled(warn); + }); + }); }); diff --git a/packages/socket-mode/src/SlackWebSocket.ts b/packages/socket-mode/src/SlackWebSocket.ts index 62f90ce82..448478f2e 100644 --- a/packages/socket-mode/src/SlackWebSocket.ts +++ b/packages/socket-mode/src/SlackWebSocket.ts @@ -18,11 +18,11 @@ interface PingPongMessage { payload: Buffer; } -function isPingPongMessage(message: unknown): message is PingPongMessage { +function isMessageForSocket(message: unknown, websocket: WebSocket): message is PingPongMessage { if (typeof message !== 'object' || message === null) { return false; } - if (!('websocket' in message && message.websocket instanceof WebSocket)) { + if (!('websocket' in message && message.websocket === websocket)) { return false; } if (!('payload' in message && Buffer.isBuffer(message.payload))) { @@ -194,29 +194,20 @@ export class SlackWebSocket { }; this.websocket.addEventListener('close', this.closeHandler); - // Subscribe to undici diagnostics_channel for WebSocket ping/pong frame events. - // These channels fire for ALL undici WebSocket instances, so we filter by matching instance. + // These channels fire for every undici WebSocket in the process, so filter to this socket's frames. this.pingHandler = (message: unknown) => { - if (!isPingPongMessage(message)) { - this.logger.warn('Received unexpected ping diagnostics message format'); - return; - } - if (message.websocket !== this.websocket) return; + if (!this.websocket || !isMessageForSocket(message, this.websocket)) return; if (this.options.pingPongLoggingEnabled) { - this.logger.debug(`WebSocket received ping from Slack server (data: ${message.payload?.toString()})`); + this.logger.debug(`WebSocket received ping from Slack server (data: ${message.payload.toString()})`); } this.monitorPingFromSlack(); }; SlackWebSocket.pingChannel.subscribe(this.pingHandler); this.pongHandler = (message: unknown) => { - if (!isPingPongMessage(message)) { - this.logger.warn('Received unexpected pong diagnostics message format'); - return; - } - if (message.websocket !== this.websocket) return; + if (!this.websocket || !isMessageForSocket(message, this.websocket)) return; if (this.options.pingPongLoggingEnabled) { - this.logger.debug(`WebSocket received pong from Slack server (data: ${message.payload?.toString()})`); + this.logger.debug(`WebSocket received pong from Slack server (data: ${message.payload.toString()})`); } this.lastPongReceivedTimestamp = Date.now(); };