From 74510b0bef3e0a936a9d2e4fad07989ef948afd2 Mon Sep 17 00:00:00 2001 From: Ben Younes Date: Sun, 13 Sep 2026 12:49:40 +0200 Subject: [PATCH] fix: retry ws opening-handshake timeout under the default ct policy (#1565) (#1576) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ws v8 library throws a plain Error('Opening handshake has timed out') with no .code and name 'Error' when the handshake timeout (JAMBONES_WS_HANDSHAKE_TIMEOUT_MS, default 1500ms) elapses. BaseRequestor._shouldRetry classified retryable errors only by .code/.name/.statusCode, so this error matched neither the ct nor rt bucket and was never retried under the default rp=ct policy — the call failed immediately on the first attempt, silently bypassing maxReconnects. Match the handshake-timeout message in the ct bucket so a transient slow WS upgrade (e.g. a serverless/edge backend cold start) is retried as intended. Adds tape coverage for both the default-ct retry path and the rp=4xx no-retry path. --- lib/utils/base-requestor.js | 11 +++- test/ws-requestor-retry-unit-test.js | 88 ++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+), 2 deletions(-) diff --git a/lib/utils/base-requestor.js b/lib/utils/base-requestor.js index 989fb765..f7851b5f 100644 --- a/lib/utils/base-requestor.js +++ b/lib/utils/base-requestor.js @@ -6,6 +6,11 @@ const timeSeries = require('@jambonz/time-series'); const {NODE_ENV, JAMBONES_TIME_SERIES_HOST} = require('../config'); let alerter ; +// The ws (v8) library throws this plain Error on an opening-handshake timeout. It carries no +// .code and .name === 'Error', so it must be matched by message to be treated as a connection +// timeout (ct) for retry purposes (issue #1565). +const WS_HANDSHAKE_TIMEOUT_MESSAGE = 'Opening handshake has timed out'; + class BaseRequestor extends Emitter { constructor(logger, account_sid, hook, secret) { super(); @@ -99,11 +104,13 @@ class BaseRequestor extends Emitter { * @returns {boolean} True if the error should be retried */ _shouldRetry(err, rpValues) { - // ct = connection timeout (ECONNREFUSED, ETIMEDOUT, etc) + // ct = connection timeout (ECONNREFUSED, ETIMEDOUT, etc). The ws opening-handshake timeout + // has no .code, so match it by message so the default ct policy retries it (issue #1565). const isCt = err.code === 'ECONNREFUSED' || err.code === 'ETIMEDOUT' || err.code === 'ECONNRESET' || - err.code === 'ECONNABORTED'; + err.code === 'ECONNABORTED' || + err.message === WS_HANDSHAKE_TIMEOUT_MESSAGE; // rt = request timeout const isRt = err.name === 'TimeoutError'; // 4xx = client errors diff --git a/test/ws-requestor-retry-unit-test.js b/test/ws-requestor-retry-unit-test.js index 5af7ee59..666c87cf 100644 --- a/test/ws-requestor-retry-unit-test.js +++ b/test/ws-requestor-retry-unit-test.js @@ -8,6 +8,9 @@ const { } = require('../lib/config'); const logger = require('pino')({level: JAMBONES_LOGLEVEL}); +// The exact message the ws v8 library throws on an opening-handshake timeout (issue #1565). +const HANDSHAKE_TIMEOUT_MESSAGE = 'Opening handshake has timed out'; + // Mock WebSocket specifically for retry testing class RetryMockWebSocket { static retryScenarios = new Map(); @@ -124,6 +127,15 @@ class RetryMockWebSocket { this.eventListeners.get('error')(err); } }); + } else if (behavior.type === 'handshake-timeout') { + // Simulate the ws v8 opening-handshake timeout: a plain Error with no + // .code property and .name === 'Error' (see issue #1565). + setImmediate(() => { + console.log(`RetryMockWebSocket: triggering handshake timeout`); + if (this.eventListeners.has('error')) { + this.eventListeners.get('error')(new Error(HANDSHAKE_TIMEOUT_MESSAGE)); + } + }); } else if (behavior.type === 'success') { // Successful connection console.log(`RetryMockWebSocket: triggering success`); @@ -572,6 +584,82 @@ test('WS Retry - rp=ct (connection timeout) should retry network errors', async t.end(); }); +test('WS Retry - handshake timeout with default (ct) policy should retry and succeed', async (t) => { + // GIVEN a handshake-timeout error (ws v8 throws a code-less Error) on the first + // attempt, and the default retry policy (no hash params => ct). Regression for #1565: + // this error type was never classified as retryable, so the call failed on attempt 1. + RetryMockWebSocket.clearScenarios(); + + const retryScenario = { + attempts: [ + { type: 'handshake-timeout' }, + { type: 'success' } + ] + }; + RetryMockWebSocket.setRetryScenario('ws://localhost:3000', retryScenario); + + const hook = { + url: 'ws://localhost:3000', // No hash parameters - defaults to ct policy + username: 'username', + password: 'password' + }; + + const params = { + callSid: 'test_handshake_timeout_ct' + }; + + // WHEN + const requestor = new WsRequestor(logger, "account_sid", hook, "webhook_secret"); + const result = await requestor.request('session:new', hook, params, {}); + + // THEN + t.ok(result, 'ws retried the handshake timeout under the default ct policy and got a response'); + t.equal(RetryMockWebSocket.getConnectionAttempts('ws://localhost:3000'), 2, + 'should have made 2 connection attempts'); + t.end(); +}); + +test('WS Retry - handshake timeout with rp=4xx should not retry', async (t) => { + // GIVEN a handshake-timeout error but a retry policy that only covers 4xx. + // Guards that the #1565 fix buckets the error as ct, not as always-retryable. + RetryMockWebSocket.clearScenarios(); + + const originalUrl = 'ws://localhost:3000#rc=2&rp=4xx'; + const cleanUrl = 'ws://localhost:3000'; + RetryMockWebSocket.setUrlMapping(cleanUrl, originalUrl); + + const retryScenario = { + attempts: [ + { type: 'handshake-timeout' } + ] + }; + RetryMockWebSocket.setRetryScenario('rc=2&rp=4xx', retryScenario); + + const hook = { + url: originalUrl, + username: 'username', + password: 'password' + }; + + const params = { + callSid: 'test_handshake_timeout_no_retry' + }; + + // WHEN & THEN + const requestor = new WsRequestor(logger, "account_sid", hook, "webhook_secret"); + try { + await requestor.request('session:new', hook, params, {}); + t.fail('Should have thrown an error'); + } catch (err) { + const errorMessage = err.message || err.toString() || String(err); + t.ok(errorMessage.includes(HANDSHAKE_TIMEOUT_MESSAGE), + 'ws properly failed without retry for handshake timeout when rp=4xx'); + t.equal(RetryMockWebSocket.getConnectionAttempts('rc=2&rp=4xx'), 1, + 'should have made only 1 connection attempt'); + t.end(); + } +}); + test('WS Retry - default behavior (no hash params) should use ct policy', async (t) => { // GIVEN RetryMockWebSocket.clearScenarios();