fix: retry transient write failures after receiving goaway

Author: ionutmanolache-okta

lib/client.js

@@ -138,8 +138,27 @@ module.exports = function (dependencies) {
     return subDirectoryObject;
   };
 
+  /**
+   * Determines if a request should be retried based on the error. This includes certain status codes, expired provider token, and transient write failures.
+   * @param {Object} error - An object representing the error which may include the following properties:
+   * @param {number} [error.status] - The HTTP status code returned from the APNs server.
+   * @param {VError} error.error - The error details which may include a message describing the error.
+   * @param {string} [error.error.message] - The error message which may indicate specific conditions such as 'ExpiredProviderToken' or transient write failures.
+   * @returns {boolean} - Returns true if the request is considered retryable based on the error, otherwise false.
+   */
+  Client.isRequestRetryable = function isRequestRetryable(error) {
+    const isStatusCodeRetryable = [408, 429, 500, 502, 503, 504].includes(error.status);
+
+    const isProviderTokenExpired =
+      error.status === 403 && error.error?.message === 'ExpiredProviderToken';
+
+    // This can happen after the server initiates a goaway, and the client did not finish closing and destroying the session yet, so the client tries to send a request on a closing session.
+    const isTransientWriteFailure = !!error.error?.message?.startsWith('apn write failed');
+
+    return isStatusCodeRetryable || isProviderTokenExpired || isTransientWriteFailure;
+  };
+
   Client.prototype.write = async function write(notification, subDirectory, type, method, count) {
-    const retryStatusCodes = [408, 429, 500, 502, 503, 504];
     const retryCount = count || 0;
     const subDirectoryLabel = this.subDirectoryLabel(type) ?? type;
     const subDirectoryInformation = this.makeSubDirectoryTypeObject(
@@ -198,21 +217,14 @@ module.exports = function (dependencies) {
         );
         return { ...subDirectoryInformation, ...sentRequest };
       } catch (error) {
-        // Determine if this is a retryable request.
-        if (
-          retryStatusCodes.includes(error.status) ||
-          (typeof error.error !== 'undefined' &&
-            error.status == 403 &&
-            error.error.message === 'ExpiredProviderToken')
-        ) {
+        if (Client.isRequestRetryable(error)) {
           try {
-            const resentRequest = await this.retryRequest(
+            const resentRequest = await this.retryWrite(
               error,
-              this.manageChannelsSession,
-              this.config.manageChannelsAddress,
               notification,
-              path,
-              httpMethod,
+              subDirectory,
+              type,
+              method,
               retryCount
             );
             return { ...subDirectoryInformation, ...resentRequest };
@@ -255,21 +267,14 @@ module.exports = function (dependencies) {
         );
         return { ...subDirectoryInformation, ...sentRequest };
       } catch (error) {
-        // Determine if this is a retryable request.
-        if (
-          retryStatusCodes.includes(error.status) ||
-          (typeof error.error !== 'undefined' &&
-            error.status == 403 &&
-            error.error.message === 'ExpiredProviderToken')
-        ) {
+        if (Client.isRequestRetryable(error)) {
           try {
-            const resentRequest = await this.retryRequest(
+            const resentRequest = await this.retryWrite(
               error,
-              this.session,
-              this.config.address,
               notification,
-              path,
-              httpMethod,
+              subDirectory,
+              type,
+              method,
               retryCount
             );
             return { ...subDirectoryInformation, ...resentRequest };
@@ -289,55 +294,24 @@ module.exports = function (dependencies) {
     }
   };
 
-  Client.prototype.retryRequest = async function retryRequest(
+  Client.prototype.retryWrite = async function retryWrite(
     error,
-    session,
-    address,
     notification,
-    path,
-    httpMethod,
-    count
+    subDirectory,
+    type,
+    method,
+    retryCount
   ) {
-    if (this.isDestroyed || session.closed) {
-      const error = { error: new VError('client session is either closed or destroyed') };
-      throw error;
-    }
-
-    const retryCount = count + 1;
-
-    if (retryCount > this.config.connectionRetryLimit) {
+    if (retryCount >= this.config.connectionRetryLimit) {
       throw error;
     }
 
     const delayInSeconds = parseInt(error.retryAfter || 0);
-    // Obey servers request to try after a specific time in ms.
     const delayPromise = new Promise(resolve => setTimeout(resolve, delayInSeconds * 1000));
     await delayPromise;
 
-    try {
-      const sentRequest = await this.request(
-        session,
-        address,
-        notification,
-        path,
-        httpMethod,
-        retryCount
-      );
-      return sentRequest;
-    } catch (error) {
-      // Recursivelly call self until retryCount is exhausted
-      // or error is thrown.
-      const sentRequest = await this.retryRequest(
-        error,
-        session,
-        address,
-        notification,
-        path,
-        httpMethod,
-        retryCount
-      );
-      return sentRequest;
-    }
+    // Retry write, which will handle reconnection if needed
+    return await this.write(notification, subDirectory, type, method, retryCount + 1);
   };
 
   Client.prototype.connect = function connect() {

test/.jshintrc

@@ -1,4 +1,5 @@
 {
+  "esversion": 11,
   "expr": true,
   "strict": false,
   "mocha": true,

test/client.js

@@ -665,7 +665,7 @@ describe('Client', () => {
     ]);
   });
 
-  it('Handles goaway frames', async () => {
+  it('Handles goaway frames and retries the request on a new connection', async () => {
     let didGetRequest = false;
     let establishedConnections = 0;
     server = createAndStartMockLowLevelServer(TEST_PORT, stream => {
@@ -714,7 +714,7 @@ describe('Client', () => {
     };
     await performRequestExpectingGoAway();
     await performRequestExpectingGoAway();
-    expect(establishedConnections).to.equal(2);
+    expect(establishedConnections).to.equal(8);
     expect(errorMessages).to.not.be.empty;
     let errorMessagesContainsGoAway = false;
     // Search for message, in older node, may be in random order.
@@ -2369,7 +2369,7 @@ describe('ManageChannelsClient', () => {
     expect(infoMessagesContainsTimeout).to.be.true;
   });
 
-  it('Handles goaway frames', async () => {
+  it('Handles goaway frames and retries the request on a new connection', async () => {
     let didGetRequest = false;
     let establishedConnections = 0;
     server = createAndStartMockLowLevelServer(TEST_PORT, stream => {
@@ -2418,7 +2418,7 @@ describe('ManageChannelsClient', () => {
     };
     await performRequestExpectingGoAway();
     await performRequestExpectingGoAway();
-    expect(establishedConnections).to.equal(2);
+    expect(establishedConnections).to.equal(8);
     expect(errorMessages).to.not.be.empty;
     let errorMessagesContainsGoAway = false;
     // Search for message, in older node, may be in random order.

test/isRequestRetryable.unit.js

@@ -0,0 +1,61 @@
+const { expect } = require('chai');
+const Client = require('../lib/client')({
+  logger: { enabled: false, log: () => {} },
+  errorLogger: { enabled: false, log: () => {} },
+  config: () => ({}),
+  http2: { constants: {} },
+});
+
+describe('Client.isRequestRetryable', () => {
+  it('returns true for error.message starting with "apn write failed"', () => {
+    const error = { error: { message: 'apn write failed: socket hang up' } };
+    expect(Client.isRequestRetryable(error)).to.be.true;
+  });
+
+  it('returns false for error.message starting with "apn write timeout" (not retryable)', () => {
+    const error = { error: { message: 'apn write timeout: timed out' } };
+    expect(Client.isRequestRetryable(error)).to.be.false;
+  });
+
+  it('returns false for error.message starting with "apn write aborted" (not retryable)', () => {
+    const error = { error: { message: 'apn write aborted: aborted' } };
+    expect(Client.isRequestRetryable(error)).to.be.false;
+  });
+
+  it('returns false for error.message with other apn messages', () => {
+    const error = { error: { message: 'apn connection closed' } };
+    expect(Client.isRequestRetryable(error)).to.be.false;
+  });
+
+  it('returns false for null error.error', () => {
+    const error = { error: null };
+    expect(Client.isRequestRetryable(error)).to.be.false;
+  });
+
+  it('returns false for undefined error.error', () => {
+    const error = {};
+    expect(Client.isRequestRetryable(error)).to.be.false;
+  });
+
+  it('returns false for error.error.message not matching', () => {
+    const error = { error: { message: 'some other error' } };
+    expect(Client.isRequestRetryable(error)).to.be.false;
+  });
+
+  it('returns true for retryable status codes', () => {
+    [408, 429, 500, 502, 503, 504].forEach(status => {
+      expect(Client.isRequestRetryable({ status })).to.be.true;
+    });
+  });
+
+  it('returns true for ExpiredProviderToken', () => {
+    const error = { status: 403, error: { message: 'ExpiredProviderToken' } };
+    expect(Client.isRequestRetryable(error)).to.be.true;
+  });
+
+  it('returns false for non-retryable status codes', () => {
+    [200, 201, 400, 404, 401, 403].forEach(status => {
+      expect(Client.isRequestRetryable({ status })).to.be.false;
+    });
+  });
+});

test/multiclient.js

@@ -463,7 +463,7 @@ describe('MultiClient', () => {
     ]);
   });
 
-  it('Handles goaway frames', async () => {
+  it('Handles goaway frames and retries the request on a new connection', async () => {
     let didGetRequest = false;
     let establishedConnections = 0;
     server = createAndStartMockLowLevelServer(TEST_PORT, stream => {
@@ -499,7 +499,7 @@ describe('MultiClient', () => {
     };
     await performRequestExpectingGoAway();
     await performRequestExpectingGoAway();
-    expect(establishedConnections).to.equal(2);
+    expect(establishedConnections).to.equal(8);
   });
 
   it('Handles unexpected protocol errors (no response sent)', async () => {
@@ -1564,7 +1564,7 @@ describe('ManageChannelsMultiClient', () => {
     ]);
   });
 
-  it('Handles goaway frames', async () => {
+  it('Handles goaway frames and retries the request on a new connection', async () => {
     let didGetRequest = false;
     let establishedConnections = 0;
     server = createAndStartMockLowLevelServer(TEST_PORT, stream => {
@@ -1600,7 +1600,7 @@ describe('ManageChannelsMultiClient', () => {
     };
     await performRequestExpectingGoAway();
     await performRequestExpectingGoAway();
-    expect(establishedConnections).to.equal(2);
+    expect(establishedConnections).to.equal(8);
   });
 
   it('Handles unexpected protocol errors (no response sent)', async () => {