From 3d728afe8f72ae8b0e8d9cf0ba0ec4f46cd9559e Mon Sep 17 00:00:00 2001 From: Ionut Manolache Date: Tue, 17 Feb 2026 20:00:30 -0500 Subject: [PATCH 1/5] 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 () => { From 5a67c8aefdd1f725e5bd1b929f6f17b4bbd6eba6 Mon Sep 17 00:00:00 2001 From: Ionut Manolache Date: Fri, 20 Feb 2026 09:39:38 -0500 Subject: [PATCH 2/5] fix: update session state management to handle closing state --- lib/client.js | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/lib/client.js b/lib/client.js index 549879ce..8dcddb25 100644 --- a/lib/client.js +++ b/lib/client.js @@ -35,6 +35,7 @@ module.exports = function (dependencies) { function Client(options) { this.isDestroyed = false; + this.isSessionClosing = false; this.config = config(options); this.logger = defaultLogger; this.errorLogger = defaultErrorLogger; @@ -91,11 +92,13 @@ module.exports = function (dependencies) { return; } if (!session.closed) { + this.isSessionClosing = true; await new Promise(resolve => { session.close(() => { resolve(); }); }); + this.isSessionClosing = false; } this.destroySession(session); }; @@ -192,6 +195,7 @@ module.exports = function (dependencies) { // Connect manageChannelsSession. if ( !this.manageChannelsSession || + this.isSessionClosing || this.manageChannelsSession.closed || this.manageChannelsSession.destroyed ) { @@ -243,7 +247,7 @@ module.exports = function (dependencies) { } } else { // Connect to standard session. - if (!this.session || this.session.closed || this.session.destroyed) { + if (!this.session || this.isSessionClosing || this.session.closed || this.session.destroyed) { try { await this.connect(); } catch (error) { @@ -345,11 +349,12 @@ module.exports = function (dependencies) { this.config )); - if (this.logger.enabled) { - this.session.on('connect', () => { + this.session.on('connect', () => { + this.isSessionClosing = false; + if (this.logger.enabled) { this.logger('Session connected'); - }); - } + } + }); this.session.on('close', () => { if (this.errorLogger.enabled) { @@ -424,11 +429,12 @@ module.exports = function (dependencies) { config )); - if (this.logger.enabled) { - this.manageChannelsSession.on('connect', () => { + this.manageChannelsSession.on('connect', () => { + this.isSessionClosing = false; + if (this.logger.enabled) { this.logger('ManageChannelsSession connected'); - }); - } + } + }); this.manageChannelsSession.on('close', () => { if (this.errorLogger.enabled) { From 2f14c4e77db928df615a80e49a767fbc2b9c8fc7 Mon Sep 17 00:00:00 2001 From: Ionut Manolache Date: Sat, 21 Feb 2026 19:54:19 -0500 Subject: [PATCH 3/5] chore: add methods to check session connection status for standard and managed channels --- lib/client.js | 35 ++++++++++++++++++++++++++++------- 1 file changed, 28 insertions(+), 7 deletions(-) diff --git a/lib/client.js b/lib/client.js index 8dcddb25..f225d005 100644 --- a/lib/client.js +++ b/lib/client.js @@ -161,6 +161,32 @@ module.exports = function (dependencies) { return isStatusCodeRetryable || isProviderTokenExpired || isTransientWriteFailure; }; + /** + * returns true if the standard session is connected and ready for use, false otherwise + * Note that even if this returns true, a request can still fail due to transient network issues, in which case the request will be retried according to the retry logic in the write method. + * @returns {boolean} + */ + Client.prototype.isStandardSessionConnected = function isStandardSessionConnected() { + return ( + this.session && !this.isSessionClosing && !this.session.closed && !this.session.destroyed + ); + }; + + /** + * returns true if the manageChannels session is connected and ready for use, false otherwise + * Note that even if this returns true, a request can still fail due to transient network issues, in which case the request will be retried according to the retry logic in the write method. + * @returns {boolean} + */ + Client.prototype.isManagedChannelsSessionConnected = + function isManagedChannelsSessionConnected() { + return ( + this.manageChannelsSession && + !this.isSessionClosing && + !this.manageChannelsSession.closed && + !this.manageChannelsSession.destroyed + ); + }; + Client.prototype.write = async function write(notification, subDirectory, type, method, count) { const retryCount = count || 0; const subDirectoryLabel = this.subDirectoryLabel(type) ?? type; @@ -193,12 +219,7 @@ module.exports = function (dependencies) { if (path.includes('/1/apps/')) { // Connect manageChannelsSession. - if ( - !this.manageChannelsSession || - this.isSessionClosing || - this.manageChannelsSession.closed || - this.manageChannelsSession.destroyed - ) { + if (!this.isManagedChannelsSessionConnected()) { try { await this.manageChannelsConnect(); } catch (error) { @@ -247,7 +268,7 @@ module.exports = function (dependencies) { } } else { // Connect to standard session. - if (!this.session || this.isSessionClosing || this.session.closed || this.session.destroyed) { + if (!this.isStandardSessionConnected()) { try { await this.connect(); } catch (error) { From 2796bf22403056a796e18414c4c0f73e847c85a6 Mon Sep 17 00:00:00 2001 From: Ionut Manolache Date: Sat, 21 Feb 2026 20:28:36 -0500 Subject: [PATCH 4/5] fix: update session closing state management to use private property --- lib/client.js | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/lib/client.js b/lib/client.js index f225d005..f1588e4d 100644 --- a/lib/client.js +++ b/lib/client.js @@ -35,7 +35,6 @@ module.exports = function (dependencies) { function Client(options) { this.isDestroyed = false; - this.isSessionClosing = false; this.config = config(options); this.logger = defaultLogger; this.errorLogger = defaultErrorLogger; @@ -92,13 +91,13 @@ module.exports = function (dependencies) { return; } if (!session.closed) { - this.isSessionClosing = true; + session._isClosing = true; await new Promise(resolve => { session.close(() => { resolve(); }); }); - this.isSessionClosing = false; + session._isClosing = false; } this.destroySession(session); }; @@ -168,7 +167,7 @@ module.exports = function (dependencies) { */ Client.prototype.isStandardSessionConnected = function isStandardSessionConnected() { return ( - this.session && !this.isSessionClosing && !this.session.closed && !this.session.destroyed + this.session && !this.session._isClosing && !this.session.closed && !this.session.destroyed ); }; @@ -181,7 +180,7 @@ module.exports = function (dependencies) { function isManagedChannelsSessionConnected() { return ( this.manageChannelsSession && - !this.isSessionClosing && + !this.manageChannelsSession._isClosing && !this.manageChannelsSession.closed && !this.manageChannelsSession.destroyed ); @@ -371,7 +370,6 @@ module.exports = function (dependencies) { )); this.session.on('connect', () => { - this.isSessionClosing = false; if (this.logger.enabled) { this.logger('Session connected'); } @@ -451,7 +449,6 @@ module.exports = function (dependencies) { )); this.manageChannelsSession.on('connect', () => { - this.isSessionClosing = false; if (this.logger.enabled) { this.logger('ManageChannelsSession connected'); } From e9481de585d804689554a989f5edb728319d43e1 Mon Sep 17 00:00:00 2001 From: Ionut Manolache Date: Sat, 21 Feb 2026 23:38:21 -0500 Subject: [PATCH 5/5] chore: improve session health check conditions for standard and managed channels --- lib/client.js | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/lib/client.js b/lib/client.js index f1588e4d..26a06b8a 100644 --- a/lib/client.js +++ b/lib/client.js @@ -39,7 +39,7 @@ module.exports = function (dependencies) { this.logger = defaultLogger; this.errorLogger = defaultErrorLogger; this.healthCheckInterval = setInterval(() => { - if (this.session && !this.session.closed && !this.session.destroyed && !this.isDestroyed) { + if (this.isStandardSessionConnected() && !this.isDestroyed) { this.session.ping((error, duration) => { if (error && this.errorLogger.enabled) { this.errorLogger( @@ -52,12 +52,7 @@ module.exports = function (dependencies) { } }, this.config.heartBeat).unref(); this.manageChannelsHealthCheckInterval = setInterval(() => { - if ( - this.manageChannelsSession && - !this.manageChannelsSession.closed && - !this.manageChannelsSession.destroyed && - !this.isDestroyed - ) { + if (this.isManagedChannelsSessionConnected() && !this.isDestroyed) { this.manageChannelsSession.ping((error, duration) => { if (error && this.errorLogger.enabled) { this.errorLogger(