fix: socket mode should warn when ack is invoked multiple times (#2519)

Co-authored-by: Eden Zimbelman <eden.zimbelman@salesforce.com>

Author: WilliamBergamin

src/receivers/HTTPResponseAck.ts

@@ -1,7 +1,7 @@
 import type { IncomingMessage, ServerResponse } from 'node:http';
 import type { Logger } from '@slack/logger';
 import { ReceiverMultipleAckError } from '../errors';
-import type { AckFn } from '../types';
+import type { AckFn, ResponseAck } from '../types';
 import * as httpFunc from './HTTPModuleFunctions';
 
 export interface AckArgs {
@@ -13,10 +13,11 @@ export interface AckArgs {
   httpResponse: ServerResponse;
 }
 
+// TODO: (semver:major) change this to "HTTPResponseBody"
 // biome-ignore lint/suspicious/noExplicitAny: response bodies can be anything
 export type HTTResponseBody = any;
 
-export class HTTPResponseAck {
+export class HTTPResponseAck implements ResponseAck {
   private logger: Logger;
 
   private isAcknowledged: boolean;

src/receivers/SocketModeReceiver.ts

@@ -21,6 +21,7 @@ import {
   type SocketModeReceiverProcessEventErrorHandlerArgs,
   defaultProcessEventErrorHandler,
 } from './SocketModeFunctions';
+import { SocketModeResponseAck } from './SocketModeResponseAck';
 import { type ReceiverRoutes, buildReceiverRoutes } from './custom-routes';
 import { verifyRedirectOpts } from './verify-redirect-opts';
 
@@ -224,10 +225,11 @@ export default class SocketModeReceiver implements Receiver {
     }
 
     this.client.on('slack_event', async (args) => {
-      const { ack, body, retry_num, retry_reason } = args;
+      const { body, retry_num, retry_reason } = args;
+      const ack = new SocketModeResponseAck({ logger: this.logger, socketModeClientAck: args.ack });
       const event: ReceiverEvent = {
         body,
-        ack,
+        ack: ack.bind(),
         retryNum: retry_num,
         retryReason: retry_reason,
         customProperties: customPropertiesExtractor(args),
@@ -241,7 +243,7 @@ export default class SocketModeReceiver implements Receiver {
           event,
         });
         if (shouldBeAcked) {
-          await ack();
+          await event.ack();
         }
       }
     });

src/receivers/SocketModeResponseAck.ts

@@ -0,0 +1,43 @@
+import type { Logger } from '@slack/logger';
+import type { AckFn, ResponseAck } from '../types';
+
+// biome-ignore lint/suspicious/noExplicitAny: response bodies can be anything
+export type SocketModeResponseBody = any;
+type SocketModeClientAck = (response?: Record<string, unknown>) => Promise<void>;
+
+export interface AckArgs {
+  logger: Logger;
+  socketModeClientAck: SocketModeClientAck;
+}
+
+export class SocketModeResponseAck implements ResponseAck {
+  private logger: Logger;
+
+  private isAcknowledged: boolean;
+
+  // TODO: the SocketModeClient should expose its send method and it should be used to acknowledge rather then an arbitrary ack() function
+  private socketModeClientAck: SocketModeClientAck;
+
+  public constructor(args: AckArgs) {
+    this.logger = args.logger;
+    this.isAcknowledged = false;
+    this.socketModeClientAck = args.socketModeClientAck;
+    // TODO: a Timeout should be used to acknowledge the request after 3 seconds and mimic the HTTPReceiver behavior
+  }
+
+  public bind(): AckFn<SocketModeResponseBody> {
+    return async (responseBody) => {
+      this.logger.debug(`ack() call begins (body: ${JSON.stringify(responseBody)})`);
+      if (this.isAcknowledged) {
+        // TODO: (semver:major) this should throw a ReceiverMultipleAckError error instead of printing a debug message
+        this.logger.warn(
+          'ack() has already been called. Additional calls will be ignored and may lead to errors in other receivers.',
+        );
+        return;
+      }
+      this.isAcknowledged = true;
+      await this.socketModeClientAck(responseBody);
+      this.logger.debug(`ack() response sent (body: ${JSON.stringify(responseBody)})`);
+    };
+  }
+}

src/types/receiver.ts

@@ -30,3 +30,9 @@ export interface Receiver {
   // biome-ignore lint/suspicious/noExplicitAny: different receivers may have different types of arguments
   stop(...args: any[]): Promise<unknown>;
 }
+
+export interface ResponseAck {
+  // Returns the function to acknowledge incoming requests
+  // biome-ignore lint/suspicious/noExplicitAny: TODO: Make the argument type more specific
+  bind(): AckFn<any>;
+}

test/unit/receivers/HTTPResponseAck.spec.ts

@@ -1,24 +1,27 @@
 import { IncomingMessage, ServerResponse } from 'node:http';
 import { assert } from 'chai';
 import sinon from 'sinon';
+import { expectType } from 'tsd';
 import { ReceiverMultipleAckError } from '../../../src/errors';
 import * as HTTPModuleFunctions from '../../../src/receivers/HTTPModuleFunctions';
 import { HTTPResponseAck } from '../../../src/receivers/HTTPResponseAck';
+import type { ResponseAck } from '../../../src/types';
 import { createFakeLogger } from '../helpers';
 
 describe('HTTPResponseAck', async () => {
-  it('should work', async () => {
+  it('should implement ResponseAck and work', async () => {
     const httpRequest = sinon.createStubInstance(IncomingMessage) as IncomingMessage;
     const httpResponse: ServerResponse = sinon.createStubInstance(ServerResponse) as unknown as ServerResponse;
-    const ack = new HTTPResponseAck({
+    const responseAck = new HTTPResponseAck({
       logger: createFakeLogger(),
       processBeforeResponse: false,
       httpRequest,
       httpResponse,
     });
-    assert.isDefined(ack);
-    assert.isDefined(ack.bind());
-    ack.ack(); // no exception
+    assert.isDefined(responseAck);
+    assert.isDefined(responseAck.bind());
+    expectType<ResponseAck>(responseAck);
+    responseAck.ack(); // no exception
   });
   it('should trigger unhandledRequestHandler if unacknowledged', (done) => {
     const httpRequest = sinon.createStubInstance(IncomingMessage) as IncomingMessage;
@@ -58,14 +61,14 @@ describe('HTTPResponseAck', async () => {
   it('should throw an error if a bound Ack invocation was already acknowledged', async () => {
     const httpRequest = sinon.createStubInstance(IncomingMessage) as IncomingMessage;
     const httpResponse: ServerResponse = sinon.createStubInstance(ServerResponse) as unknown as ServerResponse;
-    const ack = new HTTPResponseAck({
+    const responseAck = new HTTPResponseAck({
       logger: createFakeLogger(),
       processBeforeResponse: false,
       httpRequest,
       httpResponse,
     });
-    const bound = ack.bind();
-    ack.ack();
+    const bound = responseAck.bind();
+    responseAck.ack();
     try {
       await bound();
       assert.fail('No exception raised');
@@ -76,31 +79,31 @@ describe('HTTPResponseAck', async () => {
   it('should store response body if processBeforeResponse=true', async () => {
     const httpRequest = sinon.createStubInstance(IncomingMessage) as IncomingMessage;
     const httpResponse: ServerResponse = sinon.createStubInstance(ServerResponse) as unknown as ServerResponse;
-    const ack = new HTTPResponseAck({
+    const responseAck = new HTTPResponseAck({
       logger: createFakeLogger(),
       processBeforeResponse: true,
       httpRequest,
       httpResponse,
     });
-    const bound = ack.bind();
+    const bound = responseAck.bind();
     const body = { some: 'thing' };
     await bound(body);
-    assert.equal(ack.storedResponse, body, 'Body passed to bound handler not stored in Ack instance.');
+    assert.equal(responseAck.storedResponse, body, 'Body passed to bound handler not stored in Ack instance.');
   });
   it('should store an empty string if response body is falsy and processBeforeResponse=true', async () => {
     const httpRequest = sinon.createStubInstance(IncomingMessage) as IncomingMessage;
     const httpResponse: ServerResponse = sinon.createStubInstance(ServerResponse) as unknown as ServerResponse;
-    const ack = new HTTPResponseAck({
+    const responseAck = new HTTPResponseAck({
       logger: createFakeLogger(),
       processBeforeResponse: true,
       httpRequest,
       httpResponse,
     });
-    const bound = ack.bind();
+    const bound = responseAck.bind();
     const body = false;
     await bound(body);
     assert.equal(
-      ack.storedResponse,
+      responseAck.storedResponse,
       '',
       'Falsy body passed to bound handler not stored as empty string in Ack instance.',
     );
@@ -109,13 +112,13 @@ describe('HTTPResponseAck', async () => {
     const stub = sinon.stub(HTTPModuleFunctions, 'buildContentResponse');
     const httpRequest = sinon.createStubInstance(IncomingMessage) as IncomingMessage;
     const httpResponse: ServerResponse = sinon.createStubInstance(ServerResponse) as unknown as ServerResponse;
-    const ack = new HTTPResponseAck({
+    const responseAck = new HTTPResponseAck({
       logger: createFakeLogger(),
       processBeforeResponse: false,
       httpRequest,
       httpResponse,
     });
-    const bound = ack.bind();
+    const bound = responseAck.bind();
     const body = { some: 'thing' };
     await bound(body);
     assert(

test/unit/receivers/SocketModeReceiver.spec.ts

@@ -657,9 +657,8 @@ describe('SocketModeReceiver', () => {
   });
 
   describe('event', () => {
-    it('acknowledges processed events', async () => {
-      const processStub = sinon.stub<[ReceiverEvent]>().resolves();
-      processStub.callsFake(async (event: ReceiverEvent) => {
+    it('should allow events processed to be acknowledged', async () => {
+      const processStub = sinon.stub<[ReceiverEvent]>().callsFake(async (event: ReceiverEvent) => {
         await event.ack();
       });
       const app = sinon.createStubInstance(App, { processEvent: processStub }) as unknown as App;
@@ -668,17 +667,17 @@ describe('SocketModeReceiver', () => {
       receiver.init(app);
 
       // Stub ack with an awaited promise for tests
-      let resolve: () => void;
-      const called = new Promise<void>((r) => {
-        resolve = r;
+      let notifyAckCalled: () => void;
+      const ackCalled = new Promise<void>((resolve) => {
+        notifyAckCalled = resolve;
       });
       const ack = sinon.stub().callsFake(async () => {
-        resolve();
+        notifyAckCalled();
       });
 
       receiver.client.emit('slack_event', { ack });
-      await called;
-      assert.isTrue(ack.called);
+      await ackCalled;
+      sinon.assert.calledOnce(ack);
     });
 
     it('acknowledges events that throw AuthorizationError', async () => {
@@ -689,17 +688,17 @@ describe('SocketModeReceiver', () => {
       receiver.init(app);
 
       // Stub ack with an awaited promise for tests
-      let resolve: () => void;
-      const called = new Promise<void>((r) => {
-        resolve = r;
+      let notifyAckCalled: () => void;
+      const ackCalled = new Promise<void>((resolve) => {
+        notifyAckCalled = resolve;
       });
       const ack = sinon.stub().callsFake(async () => {
-        resolve();
+        notifyAckCalled();
       });
 
       receiver.client.emit('slack_event', { ack });
-      await called;
-      assert.isTrue(ack.called);
+      await ackCalled;
+      sinon.assert.calledOnce(ack);
     });
 
     it('does not acknowledge events that throw unknown errors', async () => {
@@ -714,13 +713,37 @@ describe('SocketModeReceiver', () => {
       receiver.init(app);
 
       const slackRequestArgs = { body: {}, ack: sinon.fake() };
-
       receiver.client.emit('slack_event', slackRequestArgs);
+
       while (!defaultProcessEventErrorHandlerSpy.called) {
         await delay(50);
       }
-
+      sinon.assert.calledOnce(defaultProcessEventErrorHandlerSpy);
       sinon.assert.notCalled(slackRequestArgs.ack);
     });
+
+    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');
+      });
+      const defaultProcessEventErrorHandlerSpy = sinon.spy(defaultProcessEventErrorHandler);
+      const app = sinon.createStubInstance(App, { processEvent: processStub }) as unknown as App;
+      const SocketModeReceiver = await importSocketModeReceiver(overrides);
+      const receiver = new SocketModeReceiver({
+        appToken: 'xapp-example',
+        processEventErrorHandler: defaultProcessEventErrorHandlerSpy,
+      });
+      receiver.init(app);
+
+      const slackRequestArgs = { body: {}, ack: sinon.fake() };
+      receiver.client.emit('slack_event', slackRequestArgs);
+
+      while (!defaultProcessEventErrorHandlerSpy.called) {
+        await delay(50);
+      }
+      sinon.assert.calledOnce(defaultProcessEventErrorHandlerSpy);
+      sinon.assert.calledOnce(slackRequestArgs.ack);
+    });
   });
 });

test/unit/receivers/SocketModeResponseAck.spec.ts

@@ -0,0 +1,55 @@
+import { assert } from 'chai';
+import sinon from 'sinon';
+import { expectType } from 'tsd';
+import { SocketModeResponseAck } from '../../../src/receivers/SocketModeResponseAck';
+import type { ResponseAck } from '../../../src/types';
+import { createFakeLogger } from '../helpers';
+
+describe('SocketModeResponseAck', async () => {
+  const fakeSocketModeClientAck = sinon.fake();
+  const fakeLogger = createFakeLogger();
+
+  beforeEach(() => {
+    sinon.reset();
+  });
+
+  it('should implement ResponseAck', async () => {
+    const responseAck = new SocketModeResponseAck({
+      logger: fakeLogger,
+      socketModeClientAck: fakeSocketModeClientAck,
+    });
+    assert.isDefined(responseAck);
+    assert.isDefined(responseAck.bind());
+    expectType<ResponseAck>(responseAck);
+  });
+
+  describe('bind', async () => {
+    it('should create bound Ack that invoke the response to the request', async () => {
+      const responseAck = new SocketModeResponseAck({
+        logger: fakeLogger,
+        socketModeClientAck: fakeSocketModeClientAck,
+      });
+      const ack = responseAck.bind();
+      await ack(); // no exception
+      sinon.assert.calledOnce(fakeSocketModeClientAck);
+    });
+
+    it('should log an error message when there are more then 1 bound Ack invocation', async () => {
+      const responseAck = new SocketModeResponseAck({
+        logger: fakeLogger,
+        socketModeClientAck: fakeSocketModeClientAck,
+      });
+      const ack = responseAck.bind();
+      await ack();
+      sinon.assert.neverCalledWith(
+        fakeLogger.warn,
+        'ack() has already been called. Additional calls will be ignored and may lead to errors in other receivers.',
+      );
+      await ack();
+      sinon.assert.calledWith(
+        fakeLogger.warn,
+        'ack() has already been called. Additional calls will be ignored and may lead to errors in other receivers.',
+      );
+    });
+  });
+});