From a91880ad922d2e4f2e8272a37606daf17c1b09a7 Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=B4me=20Benoit?= Date: Sun, 5 Jul 2026 20:30:45 +0200 Subject: [PATCH] fix(utils): clamp computeExponentialBackOffDelay output to setTimeout-safe range (#1951) MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Fix a latent overflow bug in `computeExponentialBackOffDelay` that turned intended exponential backoff into a tight-loop retry storm at high retry counts. The function returned `baseDelayMs * 2^retry + jitter` unclamped. With `baseDelayMs = 100` (default) it overflows `MAX_SETINTERVAL_DELAY_MS` (2_147_483_647 ms, ~24.8 days) at retry ≥25; with `baseDelayMs = 60_000` (BootNotification interval default) it overflows at retry ≥16. Node.js `setTimeout` silently coerces out-of-range delays to 1 ms per its docs, producing a tight-loop reconnect/retry storm instead of the intended exponential wait. Fix: wrap the return value with `clampToSafeTimerValue()` inside `computeExponentialBackOffDelay`. Single-site DRY defense covering all 5 present and future callers. Call sites verified (5): - ChargingStation.ts:1608 getReconnectDelay — vulnerable (wsConnectionRetryCount unbounded when autoReconnectMaxRetries = -1) - ChargingStation.ts:2532 onOpen BootNotification retry — vulnerable (registrationRetryCount unbounded when registrationMaxRetries = -1) - ChargingStation.ts:2788 sendMessageBuffer — vulnerable (messageIdx unbounded) - OCPP20ServiceUtils.ts:253 computeReconnectDelay — safe by construction (maxRetries: RetryBackOffRepeatTimes) - OCPP20CertSigningRetryManager.ts:96 scheduleNextRetry — safe by construction (maxRetries: CertSigningRepeatTimes) Extends `computeExponentialBackOffDelay` JSDoc `@returns` to advertise the clamp guarantee explicitly: `delay in milliseconds, guaranteed within [0, Constants.MAX_SETINTERVAL_DELAY_MS]`. Regression tests added at retry 25 (base 100 ms), retry 30 (extreme), and a 20-sample jitter loop at retry 25 with `jitterPercent: 0.2`. At retry ≥25 the pre-jitter delay already exceeds the ceiling, so the jitter loop asserts strict equality with MAX (deterministic). Priority-3 anchor comments preserved: mathematical formula anchor for the overflow boundary and clamp-after-jitter ordering invariant. Quality gates: typecheck clean, lint clean, tests/utils/Utils.test.ts 54/54 pass. Diff: +34/-3 across 2 files. Reviews applied (3 rounds, 4 agents each): 8 substantive findings across v1+v2 addressed; v3 saturation HIGH declared by 4/4 agents (0 substantive findings, 6 false positives rejected via caller-trace). --- src/utils/Utils.ts | 7 ++++--- tests/utils/Utils.test.ts | 30 ++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/src/utils/Utils.ts b/src/utils/Utils.ts index b8212e7a..1f94218c 100644 --- a/src/utils/Utils.ts +++ b/src/utils/Utils.ts @@ -418,14 +418,15 @@ export const insertAt = (str: string, subStr: string, pos: number): string => `${str.slice(0, pos)}${subStr}${str.slice(pos)}` /** - * Generalized exponential back-off: baseDelayMs × 2^min(retryNumber, maxRetries) + jitter. + * Generalized exponential back-off: baseDelayMs × 2^min(retryNumber, maxRetries) + jitter, + * clamped to [0, Constants.MAX_SETINTERVAL_DELAY_MS] for setTimeout/setInterval safety. * @param options - back-off configuration * @param options.baseDelayMs - base delay in milliseconds * @param options.retryNumber - current retry attempt (0-based) * @param options.maxRetries - stop doubling after this many retries (default: unlimited) * @param options.jitterMs - maximum fixed random jitter in milliseconds (default: 0) * @param options.jitterPercent - proportional jitter as fraction of computed delay, e.g. 0.2 = 20% (default: 0) - * @returns delay in milliseconds + * @returns delay in milliseconds, guaranteed within [0, Constants.MAX_SETINTERVAL_DELAY_MS] */ export const computeExponentialBackOffDelay = (options: { baseDelayMs: number @@ -443,7 +444,7 @@ export const computeExponentialBackOffDelay = (options: { } else if (jitterMs != null && jitterMs > 0) { jitter = secureRandom() * jitterMs } - return delay + jitter + return clampToSafeTimerValue(delay + jitter) } /** diff --git a/tests/utils/Utils.test.ts b/tests/utils/Utils.test.ts index 068320e3..fae297f4 100644 --- a/tests/utils/Utils.test.ts +++ b/tests/utils/Utils.test.ts @@ -706,6 +706,36 @@ await describe('Utils', async () => { const maxDelay = computeExponentialBackOffDelay({ baseDelayMs, retryNumber: 10 }) assert.strictEqual(maxDelay, 102400) // ~102 seconds }) + + await it('should clamp overflowing delays to MAX_SETINTERVAL_DELAY_MS', () => { + // At retry 25 with base 100ms: 100 * 2^25 = 3_355_443_200 ms > MAX_SETINTERVAL_DELAY_MS + // Without clamping, setTimeout would silently coerce to 1 ms (Node.js docs), producing + // a tight-loop reconnect storm instead of the intended exponential backoff. + const overflowDelay = computeExponentialBackOffDelay({ + baseDelayMs: 100, + retryNumber: 25, + }) + assert.strictEqual(overflowDelay, Constants.MAX_SETINTERVAL_DELAY_MS) + + const extremeDelay = computeExponentialBackOffDelay({ + baseDelayMs: 100, + retryNumber: 30, + }) + assert.strictEqual(extremeDelay, Constants.MAX_SETINTERVAL_DELAY_MS) + + // Clamping applies AFTER jitter is added — jitter cannot push past the safe ceiling. + // At retry 25 with base 100ms, `delay = 100 * 2^25 = 3.35e9` already exceeds the ceiling + // before jitter, so every iteration pins exactly to MAX_SETINTERVAL_DELAY_MS regardless + // of the random draw. + for (let i = 0; i < 20; i++) { + const jitteredDelay = computeExponentialBackOffDelay({ + baseDelayMs: 100, + jitterPercent: 0.2, + retryNumber: 25, + }) + assert.strictEqual(jitteredDelay, Constants.MAX_SETINTERVAL_DELAY_MS) + } + }) }) await it('should return timestamped log prefix with optional string', () => { -- 2.53.0