From 3d728afe8f72ae8b0e8d9cf0ba0ec4f46cd9559e Mon Sep 17 00:00:00 2001 From: Ionut Manolache Date: Tue, 17 Feb 2026 20:00:30 -0500 Subject: [PATCH] fix: retry transient write failures after receiving goaway --- lib/client.js | 102 ++++++++++++-------------------- test/.jshintrc | 1 + test/client.js | 8 +-- test/isRequestRetryable.unit.js | 61 +++++++++++++++++++ test/multiclient.js | 8 +-- 5 files changed, 108 insertions(+), 72 deletions(-) create mode 100644 test/isRequestRetryable.unit.js diff --git a/lib/client.js b/lib/client.js index 8703e9b1..549879ce 100644 --- a/lib/client.js +++ b/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() { diff --git a/test/.jshintrc b/test/.jshintrc index 01ad0ba5..c8769a60 100644 --- a/test/.jshintrc +++ b/test/.jshintrc @@ -1,4 +1,5 @@ { + "esversion": 11, "expr": true, "strict": false, "mocha": true, diff --git a/test/client.js b/test/client.js index 06902ca1..3f9988fd 100644 --- a/test/client.js +++ b/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. diff --git a/test/isRequestRetryable.unit.js b/test/isRequestRetryable.unit.js new file mode 100644 index 00000000..46b5b460 --- /dev/null +++ b/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; + }); + }); +}); diff --git a/test/multiclient.js b/test/multiclient.js index 80d46a18..9d6055ca 100644 --- a/test/multiclient.js +++ b/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 () => {