fix: socket mode should warn when ack is invoked multiple times by WilliamBergamin · Pull Request #2519 · slackapi/bolt-js

WilliamBergamin · 2025-05-07T16:00:18Z

Summary

The HTTPReceiver and ExpressReceiver throw a ReceiverMultipleAckError if the ack utility function is invoked more then once, but the SocketModeReceiver allows for multiple ack invocations. We recommend using SocketModeReceiver for development and the HTTPReceiver, a developer could accidentally invoke ack more then once in development and then see an error raised in production.

These changes improve the behavior of SocketModeReceiver to warn the developer whenever ack is invoked more then once. These changes also aim to align the SocketModeReceiver closer with the design patterns found in the HTTPReceiver.

NOTE: opted to warn the developer instead of throwing a ReceiverMultipleAckError since it would has cause a breaking change. We could also inform the developer through a debug log.

Testing

You can use the following app to test this behavior

{
    "display_information": {
        "name": "hello-cmd"
    },
    "features": {
        "app_home": {
            "home_tab_enabled": false,
            "messages_tab_enabled": true,
            "messages_tab_read_only_enabled": false
        },
        "bot_user": {
            "display_name": "hello-cmd",
            "always_online": false
        },
        "slash_commands": [
            {
                "command": "/hello",
                "description": "Say hello"
            }
        ]
    },
    "oauth_config": {
        "scopes": {
            "bot": [
                "commands",
                "chat:write"
            ]
        }
    },
    "settings": {
        "event_subscriptions": {
            "bot_events": []
        },
        "interactivity": {
            "is_enabled": true
        },
        "org_deploy_enabled": true,
        "socket_mode_enabled": true,
        "token_rotation_enabled": false,
    }
}

import { App, type BlockAction, LogLevel } from '@slack/bolt';

const app = new App({
  token: process.env.SLACK_BOT_TOKEN,
  socketMode: true,
  appToken: process.env.SLACK_APP_TOKEN,
  logLevel: LogLevel.DEBUG,
});

app.command('/hello', async ({ command, ack, respond }) => {
  await ack();
  await ack(); // This will warn you that you should not do this

  await respond({
    response_type: 'in_channel', // or 'ephemeral'
    text: `Hello, <@${command.user_id}>!`,
  });
});

(async () => {
  try {
    await app.start();
    app.logger.info('⚡️ Bolt app is running!');
  } catch (error) {
    app.logger.error('Failed to start the app', error);
  }
})();

Results

With log level INFO

[INFO]  bolt-app ⚡️ Bolt app is running!
[WARN]   ack() has already been invoked; subsequent calls have no effect

With log level DEBUG

[INFO]  bolt-app ⚡️ Bolt app is running!
[DEBUG]  socket-mode:SocketModeClient:0 Received a message on the WebSocket: {"envelope_id":...
[DEBUG]  web-api:WebClient:2 initialized
[DEBUG]   ack() call begins (body: undefined)
[DEBUG]  socket-mode:SocketModeClient:0 Calling ack() - type: slash_commands, envelope_id: 319b046d-745f-44d2-a18f-04410ba19e4b, data: undefined
[DEBUG]  socket-mode:SocketModeClient:0 send() method was called (WebSocket state: OPEN)
[DEBUG]  socket-mode:SlackWebSocket:1 isActive(): websocket ready state is OPEN
[DEBUG]  socket-mode:SocketModeClient:0 Sending a WebSocket message: {"envelope_id":"319b046d-745f-44d2-a18f-04410ba19e4b","payload":{}}
[DEBUG]   ack() response sent (body: undefined)
[DEBUG]   ack() call begins (body: undefined)
[WARN]   ack() has already been invoked; subsequent calls have no effect

Requirements

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

codecov · 2025-05-07T16:02:21Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.28%. Comparing base (eebf16f) to head (f9a360b).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2519      +/-   ##
==========================================
+ Coverage   93.24%   93.28%   +0.04%     
==========================================
  Files          36       37       +1     
  Lines        7443     7495      +52     
  Branches      652      656       +4     
==========================================
+ Hits         6940     6992      +52     
  Misses        498      498              
  Partials        5        5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

WilliamBergamin · 2025-05-07T16:04:37Z

src/types/receiver.ts

  stop(...args: any[]): Promise<unknown>;
 }
+
+export interface ResponseAck {


I created this interface so that HTTPResponseAck and SocketModeResponseAck can implement it, but I think its publicly facing because of where it is 🤔 Not sure if there is a better place to put this or maybe we should not do this at all

Keeping just bind within this interface makes a lot of sense to me since it returns the actual AckFn 🤖

zimeg

@WilliamBergamin This is an impressive PR! 🎉

I'm a fan of these outputs matching implementations of the HTTP ack and the ideas around @slack/socket-mode being a more direct interface to Socket Mode and leaving acknowledgements as an exercise for the calling code 📚

This PR is working great for me in IRL testing, but I'm hoping we can merge the tests of #2520 before and with these changes to make sure nothing strange is happening with the updates around bind?

A few other comments are small suggestions or ideas for future changes, but nothing that blocks these changes! Great stuff all around 🚢 💨

src/receivers/SocketModeReceiver.ts

zimeg · 2025-05-08T07:25:59Z

src/types/receiver.ts

+  bind(): AckFn<any>;
+  // The function to acknowledge incoming requests
+  // biome-ignore lint/suspicious/noExplicitAny: TODO: Make the argument type more specific
+  ack(response?: any): void | Promise<void>;


📝 The asynchronous ack implementation - where the actual response is sent - for the Socket Mode receiver makes a lot of sense to me!

I'm wondering if most of the acknowledgement logic should be moved here for the HTTP receiver too?

This might not be causing a problem in calling code, but it's a little confusing to me that the response isn't sent via ack here:

bolt-js/src/receivers/HTTPResponseAck.ts

Lines 65 to 93 in ee8df73

public bind(): AckFn<HTTResponseBody> {

return async (responseBody) => {

this.logger.debug(`ack() call begins (body: ${responseBody})`);

if (this.isAcknowledged) {

throw new ReceiverMultipleAckError();

}

this.ack();

if (this.processBeforeResponse) {

// In the case where processBeforeResponse: true is enabled,

// we don't send the HTTP response immediately. We hold off until the listener execution is completed.

if (!responseBody) {

this.storedResponse = '';

} else {

this.storedResponse = responseBody;

}

this.logger.debug(`ack() response stored (body: ${responseBody})`);

} else {

httpFunc.buildContentResponse(this.httpResponse, responseBody);

this.logger.debug(`ack() response sent (body: ${responseBody})`);

}

};

}

public ack(): void {

this.isAcknowledged = true;

if (this.noAckTimeoutId) {

clearTimeout(this.noAckTimeoutId);

}

}

🤔 I'm now wondering if bind should be a simple wrapper that calls ack where all of the actual logic would be for both of these receivers?

This might help keep logs together if ack is called without bind 🌳

🗣️ Rambling now, but this might be a change or fix better left for a follow up if it does make sense...

From my understanding ack in HTTPResponseAck does not actually build the content response because this actually fires off the response to Slack.

In the case where the handler is responsible to call ack but it throws an error then their can be a race condition where ack is called twice

See this comment

But this does make me think that this implementation should be tweaked slightly 🤔

test/unit/receivers/SocketModeResponseAck.spec.ts

src/receivers/SocketModeResponseAck.ts

test/unit/receivers/SocketModeResponseAck.spec.ts

src/receivers/HTTPResponseAck.ts

Co-authored-by: Eden Zimbelman <[email protected]>

….com/slackapi/bolt-js into socket-mode-should-warn-on-multi-ack

zimeg

@WilliamBergamin All looks good to me!

I noticed these logs are output as errors which might cause confusion when the log level is set to ERROR but the changes are working well for me overall 👾

zimeg · 2025-05-09T18:30:34Z

test/unit/receivers/SocketModeReceiver.spec.ts

+    it('does not re-acknowledge events that handle acknowledge and then throw unknown errors', async () => {
+      const processStub = sinon.stub<[ReceiverEvent]>().callsFake(async (event: ReceiverEvent) => {
+        await event.ack();
+        throw new Error('internal error');
+      });


🤩 Wonderful testcase to have! Thanks for making these improvements and tests read better overall!

zimeg · 2025-05-09T18:31:32Z

src/types/receiver.ts

  stop(...args: any[]): Promise<unknown>;
 }
+
+export interface ResponseAck {


Keeping just bind within this interface makes a lot of sense to me since it returns the actual AckFn 🤖

src/receivers/SocketModeResponseAck.ts

Co-authored-by: Eden Zimbelman <[email protected]>

WilliamBergamin added 3 commits May 6, 2025 16:45

end of day commit

43eeca1

It all works as expected

e895e28

make it work well

4a6bf36

WilliamBergamin added this to the 4.3.1 milestone May 7, 2025

WilliamBergamin requested review from hello-ashleyintech, mwbrooks and zimeg May 7, 2025 16:00

WilliamBergamin self-assigned this May 7, 2025

WilliamBergamin added enhancement M-T: A feature request for new functionality semver:minor javascript Pull requests that update Javascript code labels May 7, 2025

Merge branch 'main' into socket-mode-should-warn-on-multi-ack

0acd9f0

WilliamBergamin commented May 7, 2025

View reviewed changes

WilliamBergamin modified the milestones: 4.3.1, 4.4.0 May 7, 2025

zimeg mentioned this pull request May 8, 2025

test: confirm socket mode receiver acknowledges processed events #2520

Merged

2 tasks

zimeg approved these changes May 8, 2025

View reviewed changes

WilliamBergamin and others added 7 commits May 9, 2025 09:49

Clean up slightly

2c6839f

Update src/receivers/HTTPResponseAck.ts

8b76e50

Co-authored-by: Eden Zimbelman <[email protected]>

Merge branch 'main' into socket-mode-should-warn-on-multi-ack

ad44138

Await ack!

b7f267e

Merge branch 'socket-mode-should-warn-on-multi-ack' of https://github…

b7d8e31

….com/slackapi/bolt-js into socket-mode-should-warn-on-multi-ack

Merge branch 'main' into socket-mode-should-warn-on-multi-ack

858830b

Clean up based on feedback

68ecb4a

WilliamBergamin requested a review from zimeg May 9, 2025 17:20

Improve tests slightly

b7c8b44

zimeg approved these changes May 9, 2025

View reviewed changes

WilliamBergamin and others added 2 commits May 9, 2025 15:25

Update src/receivers/SocketModeResponseAck.ts

2e0fe52

Co-authored-by: Eden Zimbelman <[email protected]>

fix tests based on feedback

f9a360b

WilliamBergamin merged commit 691f481 into main May 9, 2025
16 checks passed

WilliamBergamin deleted the socket-mode-should-warn-on-multi-ack branch May 9, 2025 19:39

zimeg mentioned this pull request May 13, 2025

feat: support for synchronous custom step handling #2408

Merged

4 tasks

WilliamBergamin mentioned this pull request May 14, 2025

feat: expose auto acknowledgement flag on the function handler #2334

Closed

2 tasks

	public bind(): AckFn<HTTResponseBody> {
	return async (responseBody) => {
	this.logger.debug(`ack() call begins (body: ${responseBody})`);
	if (this.isAcknowledged) {
	throw new ReceiverMultipleAckError();
	}
	this.ack();
	if (this.processBeforeResponse) {
	// In the case where processBeforeResponse: true is enabled,
	// we don't send the HTTP response immediately. We hold off until the listener execution is completed.
	if (!responseBody) {
	this.storedResponse = '';
	} else {
	this.storedResponse = responseBody;
	}
	this.logger.debug(`ack() response stored (body: ${responseBody})`);
	} else {
	httpFunc.buildContentResponse(this.httpResponse, responseBody);
	this.logger.debug(`ack() response sent (body: ${responseBody})`);
	}
	};
	}

	public ack(): void {
	this.isAcknowledged = true;
	if (this.noAckTimeoutId) {
	clearTimeout(this.noAckTimeoutId);
	}
	}

Comments

Conversation

WilliamBergamin commented May 7, 2025

Summary

Testing

Results

Requirements

Uh oh!

codecov bot commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zimeg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zimeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented May 7, 2025 •

edited

Loading