fix(socket-mode): open the replacement connection before closing the old one on refresh

.changeset/socket-mode-refresh-overlap.md

@@ -0,0 +1,7 @@
+---
+"@slack/socket-mode": patch
+---
+
+Fix a 5 to 12 second gap without a connection each time Slack refreshes a Socket Mode connection ([#2767](https://github.com/slackapi/node-slack-sdk/issues/2767)). On a `disconnect` message the client now opens the replacement connection right away, without a reconnect backoff, and leaves the old connection open until Slack sends it a `disconnect` message too, so payloads Slack still sends on it during the switch are received instead of dropped. Each payload is acknowledged on the connection it arrived on. While the replacement is being opened, the current connection keeps serving, and overlapping `disconnect` messages open one replacement. A replacement that fails for good rejects with its cause, as a failed reconnect does. A `link_disabled` message still closes the connection first, since a replacement can't connect while Socket Mode is off.
+
+**Note:** during each refresh, a client now holds two connections for about 60 seconds, until Slack sends the old one its own `disconnect` (`refresh_requested`, about a minute after the `warning`). Both count toward Slack's limit of 10 open Socket Mode connections per app, so apps that run many instances at once should leave room for one extra connection per instance while they refresh.

packages/socket-mode/README.md

@@ -127,13 +127,13 @@ The client's connection to Slack has a lifecycle. This means the client can be s
 
 In the table below, the client's states are listed, which are also the names of the events you can use to observe the transition to that state. The table also includes descriptions for the states and arguments that a listener would receive.
 
-| Event Name      | Arguments | Description                                                                                                                                                                                           |
-| --------------- | --------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| `connecting`    |           | The client is in the process of connecting to the platform.                                                                                                                                           |
-| `connected`     |           | The client is connected to the platform and incoming events will start being emitted.                                                                                                                 |
-| `disconnecting` |           | The client is no longer connected to the platform and cleaning up its resources. It will soon transition to `disconnected`.                                                                           |
-| `reconnecting`  |           | The client is no longer connected to the platform and cleaning up its resources. It will soon transition to `connecting`.                                                                             |
-| `disconnected`  | `(error)` | The client is not connected to the platform. This is a steady state - no attempt to connect is occurring. The `error` argument will be `undefined` when the client initiated the disconnect (normal). |
+| Event Name      | Arguments | Description                                                                                                                                                                                                    |
+| --------------- | --------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| `connecting`    |           | The client is in the process of connecting to the platform.                                                                                                                                                    |
+| `connected`     |           | The client is connected to the platform and incoming events will start being emitted.                                                                                                                          |
+| `disconnecting` |           | The client is no longer connected to the platform and cleaning up its resources. It will soon transition to `disconnected`.                                                                                    |
+| `reconnecting`  |           | The client is replacing its connection. On a Slack refresh, the current connection keeps serving after the new one is `connected`, until Slack sends it `disconnect`. It will soon transition to `connecting`. |
+| `disconnected`  | `(error)` | The client is not connected to the platform. This is a steady state - no attempt to connect is occurring. The `error` argument will be `undefined` when the client initiated the disconnect (normal).          |
 
 The client also emits events that are part of its lifecycle, but aren't states. Instead, they represent specific moments that might be helpful to your app. The following table lists these events, their description, and includes the arguments that a listener would receive.
 

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

@@ -93,6 +93,36 @@ describe('SlackWebSocket', () => {
       ws.dispatchEvent(new ErrorEvent('error', { error: new Error('boom'), message: 'boom' }));
       sinon.assert.calledOnce(discStub);
     });
+
+    it('should emit "ws_message" with the receiving SlackWebSocket, so the client can tell connections apart', () => {
+      const ws = new WSMock();
+      SlackWebSocket = 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 client = new EventEmitter();
+      const received = sandbox.spy();
+      client.on('ws_message', received);
+      const sws = new SlackWebSocket({
+        url: 'whatevs',
+        client,
+        clientPingTimeoutMS: 1,
+        serverPingTimeoutMS: 1,
+      });
+      sws.connect();
+      ws.dispatchEvent(new MessageEvent('message', { data: '{"type":"hello"}' }));
+      sinon.assert.calledOnceWithExactly(received, '{"type":"hello"}', false, sws);
+    });
   });
 
   describe('disconnect() with an unresponsive peer', () => {

packages/socket-mode/src/SlackWebSocket.ts

@@ -179,7 +179,7 @@ export class SlackWebSocket {
         return;
       }
       const isBinary = typeof event.data !== 'string';
-      this.options.client.emit('ws_message', event.data, isBinary);
+      this.options.client.emit('ws_message', event.data, isBinary, this);
     };
     this.websocket.addEventListener('message', this.messageHandler);
 

packages/socket-mode/src/SocketModeClient.test.ts

@@ -311,7 +311,7 @@ describe('SocketModeClient', () => {
           assert.strictEqual(args.type, 'events_api');
           assert.deepEqual(args.body, body);
           await args.ack({});
-          sinon.assert.calledOnceWithExactly(send, 'malformed', {});
+          sinon.assert.calledOnceWithExactly(send, 'malformed', {}, undefined);
         }
       });
 
@@ -789,6 +789,293 @@ describe('SocketModeClient', () => {
       await assert.rejects(starting, { name: 'SMConnectAbortedError' });
     });
   });
+
+  describe('server-requested disconnect (issue #2767)', () => {
+    const refreshRequested = { type: 'disconnect', reason: 'refresh_requested' };
+    const envelope = { type: 'interactive', envelope_id: 'e-1', payload: {} };
+
+    it('should open the replacement right away and leave the old connection open after it says hello', async () => {
+      const { client, error } = createClient();
+      const old = await connect(client);
+      const disconnected = sandbox.spy();
+      client.on('disconnected', disconnected);
+
+      old.receive(refreshRequested);
+      await sleep(0);
+
+      assert.strictEqual(sockets.length, 2, 'the replacement should open without waiting for a backoff');
+      assert.strictEqual(old.disconnectCalls, 0);
+
+      sockets[1].open().hello();
+      await sleep(10);
+
+      assert.strictEqual(old.disconnectCalls, 0, 'Slack may still deliver on the old connection');
+      assert.strictEqual(client.websocket, sockets[1]);
+      old.drop();
+      await sleep(10);
+      assert.strictEqual(sockets.length, 2, "the old connection's close must not trigger another reconnect");
+      sinon.assert.notCalled(disconnected);
+      sinon.assert.notCalled(error);
+    });
+
+    it('should acknowledge each envelope on the connection it arrived on', async () => {
+      const { client } = createClient();
+      const old = await connect(client);
+
+      old.receive(refreshRequested);
+      await sleep(0);
+      old.receive(envelope);
+      sockets[1].open();
+      old.receive(envelope);
+      await sleep(0);
+
+      assert.strictEqual(old.sent.length, 2);
+      assert.strictEqual(sockets[1].sent.length, 0);
+
+      sockets[1].hello();
+      sockets[1].receive(envelope);
+      old.receive(envelope);
+      await sleep(0);
+
+      assert.strictEqual(sockets[1].sent.length, 1);
+      assert.strictEqual(old.sent.length, 3);
+    });
+
+    it('should acknowledge on the current connection when the arrival connection has closed', async () => {
+      const { client } = createClient();
+      const old = await connect(client);
+      const acks: Array<() => Promise<void>> = [];
+      client.on('slash_commands', ({ ack }) => acks.push(ack));
+
+      old.receive(refreshRequested);
+      await sleep(0);
+      sockets[1].open().hello();
+      old.receive({ type: 'slash_commands', envelope_id: 'e-2', payload: {} });
+      old.drop();
+      await acks[0]();
+
+      assert.strictEqual(old.sent.length, 0);
+      assert.strictEqual(sockets[1].sent.length, 1);
+    });
+
+    it('should close the old connection at the next refresh or on disconnect()', async () => {
+      const { client } = createClient();
+      const old = await connect(client);
+      const disconnected = sandbox.spy();
+      client.on('disconnected', disconnected);
+
+      old.receive(refreshRequested);
+      await sleep(0);
+      sockets[1].open().hello();
+      assert.strictEqual(old.disconnectCalls, 0);
+      sockets[1].receive(refreshRequested);
+      await sleep(0);
+
+      assert.strictEqual(old.disconnectCalls, 1);
+      assert.strictEqual(sockets[1].disconnectCalls, 0);
+
+      await client.disconnect();
+
+      assert.strictEqual(sockets[1].disconnectCalls, 1);
+      assert.strictEqual(sockets[2].disconnectCalls, 1);
+      sinon.assert.calledOnce(disconnected);
+    });
+
+    it('should not close an old connection again after Slack closed it', async () => {
+      const { client } = createClient();
+      const old = await connect(client);
+
+      old.receive(refreshRequested);
+      await sleep(0);
+      sockets[1].open().hello();
+      old.drop();
+      await client.disconnect();
+
+      assert.strictEqual(old.disconnectCalls, 0);
+    });
+
+    it('should back off and keep the old connection serving when the replacement fails before hello', async () => {
+      const { client } = createClient({ clientPingTimeout: 1 });
+      const old = await connect(client);
+
+      old.receive(refreshRequested);
+      await sleep(0);
+      sockets[1].drop();
+      await sleep(10);
+
+      assert.strictEqual(sockets.length, 3, 'the failed replacement should be retried with the usual backoff');
+      assert.strictEqual(old.disconnectCalls, 0);
+      old.receive(envelope);
+      await sleep(0);
+      assert.strictEqual(old.sent.length, 1);
+
+      sockets[2].open().hello();
+      await sleep(0);
+
+      assert.strictEqual(old.disconnectCalls, 0);
+      assert.strictEqual(client.websocket, sockets[2]);
+    });
+
+    it('should open only the replacement when the old connection closes while its URL is fetched', async () => {
+      const { client, holdNextURL } = createClient({ clientPingTimeout: 1 });
+      const old = await connect(client);
+      const releaseURL = holdNextURL();
+      const events = { connected: sandbox.spy(), reconnecting: sandbox.spy() };
+      for (const [name, spy] of Object.entries(events)) client.on(name, spy);
+
+      old.receive(refreshRequested);
+      await sleep(0);
+      old.drop();
+      await sleep(10);
+      releaseURL();
+      await sleep(0);
+      sockets[1].open().hello();
+      await sleep(10);
+
+      assert.deepStrictEqual(
+        sockets.map((socket) => socket.isActive()),
+        [false, true],
+      );
+      assert.strictEqual(client.websocket, sockets[1]);
+      sinon.assert.calledOnce(events.connected);
+      sinon.assert.calledOnce(events.reconnecting);
+    });
+
+    it('should open one replacement when disconnect messages arrive back to back', async () => {
+      const { client } = createClient();
+      const old = await connect(client);
+      const reconnecting = sandbox.spy();
+      client.on('reconnecting', reconnecting);
+
+      old.receive({ type: 'disconnect', reason: 'warning' });
+      old.receive(refreshRequested);
+      await sleep(0);
+      sockets[1].open().hello();
+      await sleep(10);
+
+      assert.strictEqual(sockets.length, 2);
+      assert.strictEqual(client.websocket, sockets[1]);
+      sinon.assert.calledOnce(reconnecting);
+    });
+
+    it('should close the retiring connection, not open another, on its disconnect message while the replacement handshakes', async () => {
+      const { client } = createClient();
+      const old = await connect(client);
+
+      old.receive({ type: 'disconnect', reason: 'warning' });
+      await sleep(0);
+      const replacement = sockets[1].open();
+      old.receive(refreshRequested);
+      await sleep(0);
+
+      assert.strictEqual(sockets.length, 2);
+      assert.strictEqual(old.disconnectCalls, 1);
+
+      replacement.hello();
+      replacement.receive(envelope);
+      await sleep(0);
+
+      assert.strictEqual(replacement.disconnectCalls, 0);
+      assert.strictEqual(replacement.sent.length, 1);
+      assert.strictEqual(client.websocket, replacement);
+    });
+
+    it('should reject with the cause when the replacement fails for good', async () => {
+      const { client, failNextURL } = createClient();
+      const old = await connect(client);
+      const cause = new Error('invalid_auth');
+      failNextURL(cause);
+      const [onMessage] = client.listeners('ws_message');
+
+      await assert.rejects(async () => onMessage(JSON.stringify(refreshRequested), false, old), cause);
+    });
+
+    it('should close a replacement that never opened when start() replaces it', async () => {
+      const { client } = createClient();
+      const old = await connect(client);
+
+      old.receive(refreshRequested);
+      await sleep(0);
+      const restarted = client.start();
+      await sleep(0);
+      sockets[2].open().hello();
+      await restarted;
+
+      assert.strictEqual(sockets[1].disconnectCalls, 1);
+      assert.strictEqual(old.disconnectCalls, 0);
+      assert.strictEqual(client.websocket, sockets[2]);
+    });
+
+    it('should close both connections and emit disconnected once when disconnect() runs mid-refresh', async () => {
+      const { client, error } = createClient();
+      const old = await connect(client);
+      const disconnected = sandbox.spy();
+      client.on('disconnected', disconnected);
+
+      old.receive(refreshRequested);
+      await sleep(0);
+      await client.disconnect();
+      await sleep(10);
+
+      assert.strictEqual(old.disconnectCalls, 1);
+      assert.strictEqual(sockets[1].disconnectCalls, 1);
+      sinon.assert.calledOnce(disconnected);
+      sinon.assert.notCalled(error);
+    });
+
+    it('should close without reconnecting when auto-reconnect is disabled', async () => {
+      const { client } = createClient({ autoReconnectEnabled: false });
+      const old = await connect(client);
+      const disconnected = sandbox.spy();
+      client.on('disconnected', disconnected);
+
+      old.receive(refreshRequested);
+      await sleep(10);
+
+      assert.strictEqual(old.disconnectCalls, 1);
+      assert.strictEqual(sockets.length, 1);
+      sinon.assert.calledOnce(disconnected);
+    });
+
+    it('should close the connection instead of opening a replacement when the link is disabled', async () => {
+      const { client } = createClient();
+      const old = await connect(client);
+
+      old.receive({ type: 'disconnect', reason: 'link_disabled' });
+      await sleep(0);
+
+      assert.strictEqual(sockets.length, 1);
+      assert.strictEqual(old.disconnectCalls, 1);
+      await client.disconnect();
+    });
+
+    it('should ignore a disconnect message that arrives after disconnect()', async () => {
+      const { client } = createClient();
+      const old = await connect(client);
+
+      await client.disconnect();
+      old.receive(refreshRequested);
+      await sleep(10);
+
+      assert.strictEqual(sockets.length, 1);
+    });
+
+    it('should ignore a late disconnect message from a connection a later refresh closed', async () => {
+      const { client } = createClient();
+      const old = await connect(client);
+
+      old.receive(refreshRequested);
+      await sleep(0);
+      sockets[1].open().hello();
+      sockets[1].receive(refreshRequested);
+      await sleep(0);
+      sockets[2].open().hello();
+      old.receive(refreshRequested);
+      await sleep(10);
+
+      assert.strictEqual(sockets.length, 3);
+    });
+  });
 });
 
 async function sleep(ms: number): Promise<void> {