Skip to content

fix(socket-mode): observe reconnect rejections #2657

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
w3lld1 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(socket-mode): observe reconnect rejections #2657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cd518a6
fix(socket-mode): observe reconnect rejections
w3lld1 Jul 12, 2026
5c8a15d
fix(socket-mode): handle shutdown reconnect race
w3lld1 Jul 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions packages/socket-mode/src/SocketModeClient.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,42 @@ describe('SocketModeClient', () => {
it('should resolve once Disconnected state emitted');
});

describe('reconnect', () => {
it('should observe a rejected reconnect attempt', async () => {

@mwbrooks mwbrooks Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: Really appreciate having a regression test alongside the fix - it locks in the main failure mode from #2656.

const logger = new ConsoleLogger();
const errorLog = sandbox.stub(logger, 'error');
const client = new SocketModeClient({
appToken: 'xapp-',
clientPingTimeout: 1,
logger,
});
sandbox.stub(client, 'start').rejects(new Error('reconnect failed'));

client.emit('close');
await sleep(10);

sinon.assert.calledWith(errorLog, 'Failed to reconnect Socket Mode client: Error: reconnect failed');

@mwbrooks mwbrooks Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: We should add a second test case here that covers the !this.shuttingDown guard.

});

it('should not reconnect or log an error while shutting down', async () => {
const logger = new ConsoleLogger();
const errorLog = sandbox.stub(logger, 'error');
const client = new SocketModeClient({
appToken: 'xapp-',
clientPingTimeout: 1,
logger,
});
const start = sandbox.stub(client, 'start').resolves({});

client.emit('close');
await client.disconnect();
await sleep(10);

sinon.assert.notCalled(start);
sinon.assert.notCalled(errorLog);
});
});

describe('onWebSocketMessage', () => {
// While this method is protected and cannot be invoked directly, emitting the 'message' event directly invokes it
describe('slash_commands messages', () => {
Expand Down
13 changes: 10 additions & 3 deletions packages/socket-mode/src/SocketModeClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ import { SlackWebSocket, WS_READY_STATES } from './SlackWebSocket';
import type { SocketModeOptions } from './SocketModeOptions';
import { UnrecoverableSocketModeStartError } from './UnrecoverableSocketModeStartError';

class SocketModeClientShuttingDownError extends Error {}

// Lifecycle events as described in the README
enum State {
Connecting = 'connecting',
Expand Down Expand Up @@ -142,7 +144,11 @@ export class SocketModeClient extends EventEmitter {
this.on('close', () => {
// Underlying WebSocket connection was closed, possibly reconnect.
if (!this.shuttingDown && this.autoReconnectEnabled) {
this.delayReconnectAttempt(this.start);
void this.delayReconnectAttempt(this.start).catch((error) => {
if (!(error instanceof SocketModeClientShuttingDownError)) {
this.logger.error(`Failed to reconnect Socket Mode client: ${error}`);

@mwbrooks mwbrooks Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: Nice - the if (!this.shuttingDown) guard here is great. It surfaces the real reconnect failure while keeping the log quiet during a normal disconnect() race, which is the whole point of the fix. Thanks for the care.

}
});
} else {
// If reconnect is disabled or user explicitly called `disconnect()`, emit a disconnected state.
this.emit(State.Disconnected);
Expand Down Expand Up @@ -228,14 +234,15 @@ export class SocketModeClient extends EventEmitter {
this.numOfConsecutiveReconnectionFailures += 1;
const msBeforeRetry = this.clientPingTimeoutMS * this.numOfConsecutiveReconnectionFailures;
this.logger.debug(`Before trying to reconnect, this client will wait for ${msBeforeRetry} milliseconds`);
return new Promise((res, _rej) => {
return new Promise((resolve, reject) => {
setTimeout(() => {
if (this.shuttingDown) {
this.logger.debug('Client shutting down, will not attempt reconnect.');

@mwbrooks mwbrooks Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: We should handle this differently. Resolving with undefined as T works for the close-handler, but retrieveWSSURL also calls delayReconnectAttempt (line 276) and uses the result as a WebSocket URL.

If disconnect() races a retry inside retrieveWSSURL, this now returns undefined up through the recursion and start() ends up constructing new SlackWebSocket({ url: undefined, ... }). Not ideal.

A couple of options come to mind:

  1. Short-circuit earlier so we never call into start() / return a URL when we know we're shutting down. This may match the approach throughout the package.
  2. Reject with a specific "shutdown in progress" error and have callers (both the close handler and retrieveWSSURL) swallow it explicitly. That keeps the type honest and avoids the undefined-as-URL path.

reject(new SocketModeClientShuttingDownError('Socket Mode client is shutting down'));
} else {
this.logger.debug('Continuing with reconnect...');
this.emit(State.Reconnecting);
cb.apply(this).then(res);
cb.apply(this).then(resolve, reject);
}
}, msBeforeRetry);
});
Expand Down