Skip to content

Commit 8730b5e

Browse files
WilliamBergaminClaude
andauthored
fix(socket-mode): stop orphaning the close-handshake timer on disconnect (#2771) (#2777)
Co-authored-by: Claude <svc-devxp-claude@slack-corp.com>
1 parent 1fabb45 commit 8730b5e

3 files changed

Lines changed: 103 additions & 1 deletion

File tree

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
---
2+
"@slack/socket-mode": patch
3+
---
4+
5+
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.
6+
7+
- `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.
8+
- `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.

‎packages/socket-mode/src/SlackWebSocket.test.ts‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,97 @@ describe('SlackWebSocket', () => {
161161
});
162162
});
163163

164+
describe('disconnect() when the connection drops while closing (issue #2771)', () => {
165+
class UndiciLikeWS extends EventTarget {
166+
static CONNECTING = 0;
167+
static OPEN = 1;
168+
static CLOSING = 2;
169+
static CLOSED = 3;
170+
readyState = UndiciLikeWS.CONNECTING;
171+
closeCalls = 0;
172+
close() {
173+
this.closeCalls += 1;
174+
if (this.readyState === UndiciLikeWS.CONNECTING) {
175+
this.dropConnection();
176+
this.readyState = UndiciLikeWS.CLOSING;
177+
} else if (this.readyState === UndiciLikeWS.OPEN) {
178+
this.readyState = UndiciLikeWS.CLOSING;
179+
}
180+
}
181+
dropConnection() {
182+
this.readyState = UndiciLikeWS.CLOSED;
183+
this.dispatchEvent(new ErrorEvent('error', { error: new TypeError(''), message: '' }));
184+
this.dispatchEvent(new CloseEvent('close', { code: 1006, reason: '' }));
185+
}
186+
send(_data: string) {}
187+
}
188+
189+
function connect(ws: UndiciLikeWS) {
190+
const SWS = proxyquire.load('./SlackWebSocket', {
191+
undici: {
192+
WebSocket: class Fake {
193+
static CONNECTING = 0;
194+
static OPEN = 1;
195+
static CLOSING = 2;
196+
static CLOSED = 3;
197+
constructor() {
198+
// biome-ignore lint/correctness/noConstructorReturn: for test mocking purposes
199+
return ws;
200+
}
201+
},
202+
CloseEvent,
203+
ErrorEvent,
204+
MessageEvent,
205+
ping: () => {},
206+
},
207+
}).SlackWebSocket;
208+
const client = new EventEmitter();
209+
let closeEvents = 0;
210+
client.on('close', () => {
211+
closeEvents += 1;
212+
});
213+
const logger = new ConsoleLogger();
214+
const warn = sandbox.stub(logger, 'warn');
215+
sandbox.stub(logger, 'error');
216+
const sws = new SWS({
217+
url: 'ws://127.0.0.1/',
218+
client,
219+
clientPingTimeoutMS: 60000,
220+
serverPingTimeoutMS: 60000,
221+
logger,
222+
});
223+
sws.connect();
224+
return { sws, warn, closeEvents: () => closeEvents };
225+
}
226+
227+
it('should emit "close" once and not warn when the peer drops TCP after our close frame', () => {
228+
const clock = sandbox.useFakeTimers();
229+
const ws = new UndiciLikeWS();
230+
const { sws, warn, closeEvents } = connect(ws);
231+
ws.readyState = UndiciLikeWS.OPEN;
232+
233+
sws.disconnect();
234+
ws.dropConnection();
235+
clock.tick(60000);
236+
237+
assert.strictEqual(closeEvents(), 1);
238+
assert.strictEqual(ws.closeCalls, 1, 'no close frame should be sent on an already closed socket');
239+
sinon.assert.notCalled(warn);
240+
});
241+
242+
it('should emit "close" once and not warn when disconnecting before the connection opens', () => {
243+
const clock = sandbox.useFakeTimers();
244+
const ws = new UndiciLikeWS();
245+
const { sws, warn, closeEvents } = connect(ws);
246+
247+
sws.disconnect();
248+
clock.tick(60000);
249+
250+
assert.strictEqual(closeEvents(), 1);
251+
sinon.assert.notCalled(warn);
252+
});
253+
});
254+
164255
describe('cleanup() with a captured socket', () => {
165256
it('should destroy the captured underlying socket during cleanup', () => {
166257
const sws = new SlackWebSocket({

‎packages/socket-mode/src/SlackWebSocket.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -249,15 +249,18 @@ export class SlackWebSocket {
249249
} else if (this.websocket.readyState === WebSocket.CLOSING) {
250250
this.logger.debug('Terminating WebSocket (close frame sent but no response, force-terminating).');
251251
this.cleanup();
252+
} else if (this.websocket.readyState === WebSocket.CLOSED) {
253+
this.logger.debug('Terminating WebSocket (connection already closed).');
254+
this.cleanup();
252255
} else {
253256
// If we haven't received a close frame yet, then we send one to the peer, expecting to receive a close frame
254257
// in response.
255258
this.logger.debug('Sending close frame (status=1000).');
256-
this.websocket.close(1000); // 1000 = Normal Closure
257259
this.closeHandshakeTimeout = setTimeout(() => {
258260
this.logger.warn('Peer did not complete the close handshake in time; forcing cleanup.');
259261
this.cleanup();
260262
}, CLOSE_HANDSHAKE_TIMEOUT_MS);
263+
this.websocket.close(1000); // 1000 = Normal Closure
261264
}
262265
} else {
263266
this.logger.debug('WebSocket already disconnected, flushing remainder.');

0 commit comments

Comments
 (0)