From 473b857c7d7c5243c763a944c7227c8341ac4a4c Mon Sep 17 00:00:00 2001 From: William Bergamin Date: Thu, 15 Oct 2026 11:39:04 -0400 Subject: [PATCH] fix(socket-mode): stop orphaning the close-handshake timer when the socket closes mid-close (#2771) disconnect() armed a second 30s close-handshake timer when the socket was already CLOSED, losing the first. undici fires 'error' only after setting CLOSED, so a TCP drop after our close frame always hit this. The lost timer later logged a false handshake warning and emitted a second 'close', which could start a duplicate reconnect. - Clean up immediately when disconnect() finds the socket CLOSED. - Arm the timer before close(): on a CONNECTING socket undici fires 'error' and 'close' synchronously inside close(), so the re-entrant cleanup() now clears it instead of running before it is armed. Co-Authored-By: Claude --- .../socket-mode-close-handshake-timer.md | 8 ++ .../socket-mode/src/SlackWebSocket.test.ts | 91 +++++++++++++++++++ packages/socket-mode/src/SlackWebSocket.ts | 5 +- 3 files changed, 103 insertions(+), 1 deletion(-) create mode 100644 .changeset/socket-mode-close-handshake-timer.md diff --git a/.changeset/socket-mode-close-handshake-timer.md b/.changeset/socket-mode-close-handshake-timer.md new file mode 100644 index 000000000..7675011fe --- /dev/null +++ b/.changeset/socket-mode-close-handshake-timer.md @@ -0,0 +1,8 @@ +--- +"@slack/socket-mode": patch +--- + +Fix an orphaned close-handshake timer ([#2771](https://github.com/slackapi/node-slack-sdk/issues/2771)). When the connection closed while a close frame was in flight, the 30s timer armed by `disconnect()` outlived the connection, then logged a false "Peer did not complete the close handshake" warning and emitted a second `close`, which could start a duplicate reconnect. + +- `disconnect()` on an already closed socket now cleans up immediately instead of sending a close frame and arming a second timer. This is the path when Slack drops the TCP connection after a `disconnect` message. +- `disconnect()` before the connection opens no longer leaves the timer behind: undici fails a connecting socket synchronously inside `close()`, so the timer is now armed before it. diff --git a/packages/socket-mode/src/SlackWebSocket.test.ts b/packages/socket-mode/src/SlackWebSocket.test.ts index a1fde4e4f..00f0bce04 100644 --- a/packages/socket-mode/src/SlackWebSocket.test.ts +++ b/packages/socket-mode/src/SlackWebSocket.test.ts @@ -161,6 +161,97 @@ describe('SlackWebSocket', () => { }); }); + describe('disconnect() when the connection drops while closing (issue #2771)', () => { + class UndiciLikeWS extends EventTarget { + static CONNECTING = 0; + static OPEN = 1; + static CLOSING = 2; + static CLOSED = 3; + readyState = UndiciLikeWS.CONNECTING; + closeCalls = 0; + close() { + this.closeCalls += 1; + if (this.readyState === UndiciLikeWS.CONNECTING) { + this.dropConnection(); + this.readyState = UndiciLikeWS.CLOSING; + } else if (this.readyState === UndiciLikeWS.OPEN) { + this.readyState = UndiciLikeWS.CLOSING; + } + } + dropConnection() { + this.readyState = UndiciLikeWS.CLOSED; + this.dispatchEvent(new ErrorEvent('error', { error: new TypeError(''), message: '' })); + this.dispatchEvent(new CloseEvent('close', { code: 1006, reason: '' })); + } + send(_data: string) {} + } + + function connect(ws: UndiciLikeWS) { + const SWS = proxyquire.load('./SlackWebSocket', { + undici: { + WebSocket: class Fake { + static CONNECTING = 0; + static OPEN = 1; + static CLOSING = 2; + static CLOSED = 3; + constructor() { + // biome-ignore lint/correctness/noConstructorReturn: for test mocking purposes + return ws; + } + }, + CloseEvent, + ErrorEvent, + MessageEvent, + ping: () => {}, + }, + }).SlackWebSocket; + const client = new EventEmitter(); + let closeEvents = 0; + client.on('close', () => { + closeEvents += 1; + }); + const logger = new ConsoleLogger(); + const warn = sandbox.stub(logger, 'warn'); + sandbox.stub(logger, 'error'); + const sws = new SWS({ + url: 'ws://127.0.0.1/', + client, + clientPingTimeoutMS: 60000, + serverPingTimeoutMS: 60000, + logger, + }); + sws.connect(); + return { sws, warn, closeEvents: () => closeEvents }; + } + + it('should emit "close" once and not warn when the peer drops TCP after our close frame', () => { + const clock = sandbox.useFakeTimers(); + const ws = new UndiciLikeWS(); + const { sws, warn, closeEvents } = connect(ws); + ws.readyState = UndiciLikeWS.OPEN; + + sws.disconnect(); + ws.dropConnection(); + clock.tick(60000); + + assert.strictEqual(closeEvents(), 1); + assert.strictEqual(ws.closeCalls, 1, 'no close frame should be sent on an already closed socket'); + sinon.assert.notCalled(warn); + }); + + it('should emit "close" once and not warn when disconnecting before the connection opens', () => { + const clock = sandbox.useFakeTimers(); + const ws = new UndiciLikeWS(); + const { sws, warn, closeEvents } = connect(ws); + + sws.disconnect(); + clock.tick(60000); + + assert.strictEqual(closeEvents(), 1); + sinon.assert.notCalled(warn); + }); + }); + describe('cleanup() with a captured socket', () => { it('should destroy the captured underlying socket during cleanup', () => { const sws = new SlackWebSocket({ diff --git a/packages/socket-mode/src/SlackWebSocket.ts b/packages/socket-mode/src/SlackWebSocket.ts index 448478f2e..c5540197a 100644 --- a/packages/socket-mode/src/SlackWebSocket.ts +++ b/packages/socket-mode/src/SlackWebSocket.ts @@ -249,15 +249,18 @@ export class SlackWebSocket { } else if (this.websocket.readyState === WebSocket.CLOSING) { this.logger.debug('Terminating WebSocket (close frame sent but no response, force-terminating).'); this.cleanup(); + } else if (this.websocket.readyState === WebSocket.CLOSED) { + this.logger.debug('Terminating WebSocket (connection already closed).'); + this.cleanup(); } else { // If we haven't received a close frame yet, then we send one to the peer, expecting to receive a close frame // in response. this.logger.debug('Sending close frame (status=1000).'); - this.websocket.close(1000); // 1000 = Normal Closure this.closeHandshakeTimeout = setTimeout(() => { this.logger.warn('Peer did not complete the close handshake in time; forcing cleanup.'); this.cleanup(); }, CLOSE_HANDSHAKE_TIMEOUT_MS); + this.websocket.close(1000); // 1000 = Normal Closure } } else { this.logger.debug('WebSocket already disconnected, flushing remainder.');