Fix Acknowledgment Callback Signature with ackTimeout Option

packages/socket.io-client/lib/socket.ts

@@ -246,27 +246,37 @@ export class Socket<
    *
    * The `withError` attribute is used to differentiate handlers that accept an error as first argument:
    *
-   * - `socket.emit("test", (err, value) => { ... })` with `ackTimeout` option
    * - `socket.timeout(5000).emit("test", (err, value) => { ... })`
    * - `const value = await socket.emitWithAck("test")`
    *
    * From those that don't:
    *
    * - `socket.emit("test", (value) => { ... });`
+   * - `socket.emit("test", (value) => { ... })` with `ackTimeout` option (success case)
    *
-   * In the first case, the handlers will be called with an error when:
+   * The `hasTimeout` attribute indicates whether a timeout was configured (either via `.timeout()` or `ackTimeout` option).
+   * This is used for timeout handling, but NOT for disconnection handling.
+   *
+   * In the first case (withError=true), the handlers will be called with an error when:
    *
    * - the timeout is reached
    * - the socket gets disconnected
    *
-   * In the second case, the handlers will be simply discarded upon disconnection, since the client will never receive
+   * In the second case (withError=false but hasTimeout=true), the handlers will NOT be called with an error:
+   *
+   * - on timeout: callback is silently discarded (callback signature is (value) => {} and doesn't accept errors)
+   * - on disconnection: callback is silently discarded (callback signature is (value) => {} and doesn't accept errors)
+   *
+   * Successful responses will NOT prepend null (only when withError=true).
+   *
+   * In the third case (no timeout), the handlers will be simply discarded upon disconnection, since the client will never receive
    * an acknowledgement from the server.
    *
    * @private
    */
   private acks: Record<
     string,
-    ((...args: any[]) => void) & { withError?: boolean }
+    ((...args: any[]) => void) & { withError?: boolean; hasTimeout?: boolean }
   > = {};
   private flags: Flags = {};
   private subs?: Array<VoidFunction>;
@@ -463,30 +473,66 @@ export class Socket<
    */
   private _registerAckCallback(id: number, ack: (...args: any[]) => void) {
     const timeout = this.flags.timeout ?? this._opts.ackTimeout;
-    if (timeout === undefined) {
+    const hasExplicitTimeout = this.flags.timeout !== undefined;
+    const isFromQueue = this.flags.fromQueue === true;
+
+    // If packet is from queue, we need to wrap the callback even without timeout
+    // to ensure onack prepends null (via withError flag) so the queue callback
+    // receives (null, ...responseArgs) format
+    if (timeout === undefined && !isFromQueue) {
       this.acks[id] = ack;
       return;
     }
 
-    // @ts-ignore
-    const timer = this.io.setTimeoutFn(() => {
-      delete this.acks[id];
-      for (let i = 0; i < this.sendBuffer.length; i++) {
-        if (this.sendBuffer[i].id === id) {
-          debug("removing packet with ack id %d from the buffer", id);
-          this.sendBuffer.splice(i, 1);
+    // Create a wrapper function that will handle the callback
+    // Determine if withError will be set (needed for timeout handler)
+    const willHaveWithError = hasExplicitTimeout || isFromQueue;
+    let timer;
+    if (timeout !== undefined) {
+      // @ts-ignore
+      timer = this.io.setTimeoutFn(() => {
+        delete this.acks[id];
+        for (let i = 0; i < this.sendBuffer.length; i++) {
+          if (this.sendBuffer[i].id === id) {
+            debug("removing packet with ack id %d from the buffer", id);
+            this.sendBuffer.splice(i, 1);
+          }
         }
-      }
-      debug("event with ack id %d has timed out after %d ms", id, timeout);
-      ack.call(this, new Error("operation has timed out"));
-    }, timeout);
+        debug("event with ack id %d has timed out after %d ms", id, timeout);
+        // Only call with error if callback expects an error parameter (withError=true)
+        // When only ackTimeout is set (without explicit .timeout() and not from queue),
+        // the callback signature is (value) => {} and should not receive an error on timeout
+        if (willHaveWithError) {
+          ack.call(this, new Error("operation has timed out"));
+        }
+        // Otherwise, silently discard the callback (similar to disconnection behavior)
+      }, timeout);
+    }
 
     const fn = (...args: any[]) => {
-      // @ts-ignore
-      this.io.clearTimeoutFn(timer);
+      if (timer !== undefined) {
+        // @ts-ignore
+        this.io.clearTimeoutFn(timer);
+      }
+      // onack will prepend null when withError is true, so args already contains (null, ...responseArgs)
+      // for queued packets or explicit timeout cases
       ack.apply(this, args);
     };
-    fn.withError = true;
+
+    // Set hasTimeout flag when any timeout is configured (explicit or ackTimeout)
+    // This is used to determine whether to call with error upon disconnection
+    if (timeout !== undefined) {
+      fn.hasTimeout = true;
+    }
+
+    // Set withError flag when:
+    // 1. Explicitly using .timeout() - user expects (err, response) signature
+    // 2. Packet is from queue - queue's internal callback expects (err, response) signature
+    //    onack will prepend null when this flag is set
+    // Otherwise, when only ackTimeout is set, don't set withError to preserve (response) signature
+    if (hasExplicitTimeout || isFromQueue) {
+      fn.withError = true;
+    }
 
     this.acks[id] = fn;
   }
@@ -682,13 +728,19 @@ export class Socket<
         (packet) => String(packet.id) === id,
       );
       if (!isBuffered) {
-        // note: handlers that do not accept an error as first argument are ignored here
         const ack = this.acks[id];
         delete this.acks[id];
 
+        // Only call with error if the callback expects an error parameter (withError=true)
+        // When only ackTimeout is set (without explicit .timeout()), the callback signature
+        // is (value) => {} and should not receive an error on disconnection
         if (ack.withError) {
           ack.call(this, new Error("socket has been disconnected"));
         }
+        // note: handlers without withError flag are ignored here, since the client will never receive
+        // an acknowledgement from the server. This includes:
+        // - handlers without timeout
+        // - handlers with only ackTimeout (but not explicit .timeout())
       }
     });
   }

packages/socket.io-client/test/node.ts

@@ -28,12 +28,19 @@ describe("autoUnref option", function () {
   });
 
   it("should not stop with autoUnref set to false", (done) => {
+    let doneCalled = false;
     const process = exec(fixture("no-unref.ts"), () => {
-      done(new Error("should not happen"));
+      if (!doneCalled) {
+        doneCalled = true;
+        done(new Error("should not happen"));
+      }
     });
     setTimeout(() => {
       process.kill();
-      done();
+      if (!doneCalled) {
+        doneCalled = true;
+        done();
+      }
     }, 100);
   });
 });

packages/socket.io-client/test/retry.ts

@@ -146,4 +146,31 @@ describe("retry", () => {
       }, 100);
     });
   });
+
+  it("should handle queued packets without timeout correctly", () => {
+    return wrap((done) => {
+      const socket = io(BASE_URL, {
+        forceNew: true,
+        retries: 1,
+        // No ackTimeout or explicit timeout
+      });
+
+      let responseReceived = false;
+
+      socket.emit("echo", "test", (err, val) => {
+        // Queue callback expects (err, ...responseArgs) signature
+        expect(err).to.be(null);
+        expect(val).to.eql("test");
+        responseReceived = true;
+        success(done, socket);
+      });
+
+      // Wait a bit to ensure the response is processed
+      setTimeout(() => {
+        if (!responseReceived) {
+          done(new Error("Response not received"));
+        }
+      }, 500);
+    });
+  });
 });

packages/socket.io-client/test/socket.ts

@@ -657,10 +657,19 @@ describe("socket", () => {
           ackTimeout: 50,
         });
 
-        socket.emit("unknown", (err) => {
-          expect(err).to.be.an(Error);
-          success(done, socket);
+        // When only ackTimeout is set (without explicit .timeout()), the callback
+        // signature is (value) => {} and should not receive an error on timeout
+        // The timeout will silently discard the callback
+        socket.emit("unknown", (_value) => {
+          done(new Error("should not happen"));
         });
+
+        // Wait for timeout to occur
+        setTimeout(() => {
+          // @ts-ignore property 'acks' is private
+          expect(Object.keys(socket.acks).length).to.eql(0);
+          success(done, socket);
+        }, 100);
       });
     });
 
@@ -707,21 +716,26 @@ describe("socket", () => {
         });
       });
 
-      it("should ack with an error upon disconnection (callback & ackTimeout)", () => {
+      it("should not ack upon disconnection (callback & ackTimeout)", () => {
         return wrap((done) => {
           const socket = io(BASE_URL, {
             forceNew: true,
             ackTimeout: 10_000,
           });
 
           socket.on("connect", () => {
-            socket.emit("echo", "a", (err) => {
-              expect(err).to.be.an(Error);
-
-              success(done, socket);
+            // When only ackTimeout is set (without explicit .timeout()), the callback
+            // signature is (value) => {} and should not receive an error on disconnection
+            socket.emit("echo", "a", (_value) => {
+              done(new Error("should not happen"));
             });
 
             socket.disconnect();
+
+            // @ts-ignore property 'acks' is private
+            expect(Object.keys(socket.acks).length).to.eql(0);
+
+            setTimeout(() => success(done, socket), 100);
           });
         });
       });
@@ -788,6 +802,52 @@ describe("socket", () => {
     });
   });
 
+  describe("acknowledgment callback signature", () => {
+    it("should have consistent callback signature when emitting before and after connection with ackTimeout", () => {
+      return wrap((done) => {
+        const socket = io(BASE_URL, {
+          forceNew: true,
+          ackTimeout: 5000, // Set ackTimeout but don't use .timeout()
+        });
+
+        let beforeConnectArgs: any[] | null = null;
+        let afterConnectArgs: any[] | null = null;
+
+        // Emit before connection - callback should receive 1 argument (response)
+        socket.emit("echo", "test-before", function () {
+          beforeConnectArgs = Array.from(arguments);
+          // Verify callback receives only 1 argument (the response)
+          expect(beforeConnectArgs.length).to.eql(1);
+          expect(beforeConnectArgs[0]).to.eql("test-before");
+
+          // Check that after connection callback was also called
+          if (afterConnectArgs !== null) {
+            expect(afterConnectArgs.length).to.eql(1);
+            expect(afterConnectArgs[0]).to.eql("test-after");
+            success(done, socket);
+          }
+        });
+
+        socket.on("connect", () => {
+          // Emit after connection - callback should receive 1 argument (response)
+          socket.emit("echo", "test-after", function () {
+            afterConnectArgs = Array.from(arguments);
+            // Verify callback receives only 1 argument (the response)
+            expect(afterConnectArgs.length).to.eql(1);
+            expect(afterConnectArgs[0]).to.eql("test-after");
+
+            // Check that before connection callback was also called
+            if (beforeConnectArgs !== null) {
+              expect(beforeConnectArgs.length).to.eql(1);
+              expect(beforeConnectArgs[0]).to.eql("test-before");
+              success(done, socket);
+            }
+          });
+        });
+      });
+    });
+  });
+
   describe("throttled timer", () => {
     it("should buffer the event and send it upon reconnection", () => {
       return wrap((done) => {