From 2a9def4783ea64c3b356000b0e8e49a67288bdc7 Mon Sep 17 00:00:00 2001 From: apascoa Date: Wed, 2 Sep 2026 10:24:58 +0100 Subject: [PATCH 1/5] fix: stop measurements after authorization rejection --- .../BandwidthEngine/BandwidthEngine.ts | 24 +++++++++++++++++-- src/index.ts | 22 +++++++++++++++++ tests/unit/config/authorizationToken.test.ts | 15 ++++++++++++ 3 files changed, 59 insertions(+), 2 deletions(-) diff --git a/src/engines/BandwidthEngine/BandwidthEngine.ts b/src/engines/BandwidthEngine/BandwidthEngine.ts index a3353df..09a553f 100644 --- a/src/engines/BandwidthEngine/BandwidthEngine.ts +++ b/src/engines/BandwidthEngine/BandwidthEngine.ts @@ -6,6 +6,15 @@ import { const MAX_RETRIES = 20; +class HttpError extends Error { + constructor( + readonly status: number, + statusText: string + ) { + super(statusText); + } +} + const ESTIMATED_HEADER_FRACTION = 0.005; // ~.5% of packet header / payload size. used when transferSize is not available. const SERVER_TIME_MIN_DURATION = 0.01; // minimum server-provided duration value to consider valid (ms) @@ -424,7 +433,7 @@ class BandwidthMeasurementEngine implements Engine { }) .then(r => { if (r.ok) return r; - throw Error(r.statusText); + throw new HttpError(r.status, r.statusText); }) .then(r => { this.getServerTime && (serverTime = this.getServerTime(r)); @@ -567,7 +576,18 @@ class BandwidthMeasurementEngine implements Engine { } console.warn(`Error fetching ${url}: ${error}`); - if (this.#retries++ < MAX_RETRIES) { + const retryable = + !(error instanceof HttpError) || + error.status === 408 || + error.status === 429 || + (error.status >= 500 && error.status <= 599); + if (!retryable) { + this.#retries = 0; + this.#setRunning(false); + this.#onConnectionError( + `Request failed with ${error.status}: ${url}` + ); + } else if (this.#retries++ < MAX_RETRIES) { this.#nextMeasurement(); // keep trying } else { this.#retries = 0; diff --git a/src/index.ts b/src/index.ts index d38ee3a..c0cd295 100644 --- a/src/index.ts +++ b/src/index.ts @@ -75,6 +75,9 @@ type SpeedTestConfig = Config & [key: string]: unknown; }; +const isAuthorizationRejection = (error: unknown): error is string => + typeof error === 'string' && /^Request failed with (401|403):/.test(error); + /** Per-type measurement result bucket stored in Results.raw. */ interface MeasurementResult { started: boolean; @@ -154,6 +157,11 @@ class MeasurementEngine { return this.#authorization; } + /** Replaces the bearer token used by requests created after this call. */ + setAuthorizationToken(token: string | null): void { + this.#authorization.token = token; + } + /** Not paused and not finished. */ get isRunning(): boolean { return this.#running; @@ -318,6 +326,8 @@ class MeasurementEngine { } #next(): void { + if (!this.#running) return; + const resumeType = this.#curType(); const resumeResults = this.#curTypeResults(); if ( @@ -474,6 +484,10 @@ class MeasurementEngine { }; engine!.onConnectionError = (e: unknown) => { + if (isAuthorizationRejection(e)) { + this.#onError(e); + return; + } msmResults.error = e; this.onResultsChange({ type }); this.#onError(`Connection error while measuring packet loss: ${e}`); @@ -562,6 +576,10 @@ class MeasurementEngine { }; engine.onConnectionError = (e: unknown) => { + if (isAuthorizationRejection(e)) { + this.#onError(e); + return; + } this.#serverTimeDelta = (engine as BandwidthEngine).serverTimeDelta; msmResults.error = e; this.onResultsChange({ type }); @@ -717,6 +735,10 @@ class MeasurementEngine { }; engine.onConnectionError = (e: unknown) => { + if (isAuthorizationRejection(e)) { + this.#onError(e); + return; + } this.#serverTimeDelta = (engine as BandwidthEngine).serverTimeDelta; msmResults.error = e; this.onResultsChange({ type }); diff --git a/tests/unit/config/authorizationToken.test.ts b/tests/unit/config/authorizationToken.test.ts index 3ea9174..acf1541 100644 --- a/tests/unit/config/authorizationToken.test.ts +++ b/tests/unit/config/authorizationToken.test.ts @@ -11,6 +11,10 @@ class InspectableEngine extends SpeedTestEngine { get resolvedConfig() { return this.config; } + + get resolvedAuthorization() { + return this.authorization; + } } const API_URL_KEYS = [ @@ -51,6 +55,17 @@ describe('authorizationToken', () => { ).toBe(TOKEN); }); + it('replaces the token used by future requests', () => { + const engine = new InspectableEngine({ + autoStart: false, + authorizationToken: TOKEN + }); + + engine.setAuthorizationToken('replacement-token'); + + expect(engine.resolvedAuthorization.token).toBe('replacement-token'); + }); + it('defaults to null', () => { expect(resolveConfig({}).authorizationToken).toBeNull(); expect(defaultConfig.authorizationToken).toBeNull(); From dfcea420217e3a6ea846ca08250651ffcde775f6 Mon Sep 17 00:00:00 2001 From: apascoa Date: Wed, 2 Sep 2026 15:32:04 +0100 Subject: [PATCH 2/5] fix: surface terminal HTTP response statuses --- .../BandwidthEngine/BandwidthEngine.ts | 14 ++- .../BandwidthEngine/ParallelLatency.ts | 6 +- src/index.ts | 32 +++---- tests/e2e/httpError.test.ts | 88 +++++++++++++++++++ tests/unit/config/authorizationToken.test.ts | 15 ---- 5 files changed, 107 insertions(+), 48 deletions(-) create mode 100644 tests/e2e/httpError.test.ts diff --git a/src/engines/BandwidthEngine/BandwidthEngine.ts b/src/engines/BandwidthEngine/BandwidthEngine.ts index 09a553f..dcf814c 100644 --- a/src/engines/BandwidthEngine/BandwidthEngine.ts +++ b/src/engines/BandwidthEngine/BandwidthEngine.ts @@ -239,8 +239,8 @@ class BandwidthMeasurementEngine implements Engine { set onFinished(f: (results: BandwidthEngineResults) => void) { this.#onFinished = f; } - #onConnectionError: (error: string) => void = () => {}; // Invoked when unable to get a response from the API - set onConnectionError(f: (error: string) => void) { + #onConnectionError: (error: string, status?: number) => void = () => {}; // Invoked when unable to get a response from the API + set onConnectionError(f: (error: string, status?: number) => void) { this.#onConnectionError = f; } @@ -576,16 +576,12 @@ class BandwidthMeasurementEngine implements Engine { } console.warn(`Error fetching ${url}: ${error}`); - const retryable = - !(error instanceof HttpError) || - error.status === 408 || - error.status === 429 || - (error.status >= 500 && error.status <= 599); - if (!retryable) { + if (error instanceof HttpError) { this.#retries = 0; this.#setRunning(false); this.#onConnectionError( - `Request failed with ${error.status}: ${url}` + `Request failed with ${error.status}: ${url}`, + error.status ); } else if (this.#retries++ < MAX_RETRIES) { this.#nextMeasurement(); // keep trying diff --git a/src/engines/BandwidthEngine/ParallelLatency.ts b/src/engines/BandwidthEngine/ParallelLatency.ts index 6db7228..2911a77 100644 --- a/src/engines/BandwidthEngine/ParallelLatency.ts +++ b/src/engines/BandwidthEngine/ParallelLatency.ts @@ -96,8 +96,10 @@ class BandwidthWithParallelLatencyEngine extends BandwidthEngine { }; } - set onConnectionError(onConnectionError: (error: string) => void) { - super.onConnectionError = (...args: [string]) => { + set onConnectionError( + onConnectionError: (error: string, status?: number) => void + ) { + super.onConnectionError = (...args: [string, number?]) => { this.#latencyEngine && this.#latencyEngine.pause(); onConnectionError(...args); }; diff --git a/src/index.ts b/src/index.ts index c0cd295..b85262e 100644 --- a/src/index.ts +++ b/src/index.ts @@ -75,9 +75,6 @@ type SpeedTestConfig = Config & [key: string]: unknown; }; -const isAuthorizationRejection = (error: unknown): error is string => - typeof error === 'string' && /^Request failed with (401|403):/.test(error); - /** Per-type measurement result bucket stored in Results.raw. */ interface MeasurementResult { started: boolean; @@ -157,11 +154,6 @@ class MeasurementEngine { return this.#authorization; } - /** Replaces the bearer token used by requests created after this call. */ - setAuthorizationToken(token: string | null): void { - this.#authorization.token = token; - } - /** Not paused and not finished. */ get isRunning(): boolean { return this.#running; @@ -183,9 +175,9 @@ class MeasurementEngine { this.#onFinish = f; } - #onError: (message: string) => void = () => {}; + #onError: (message: string, status?: number) => void = () => {}; - set onError(f: (message: string) => void) { + set onError(f: (message: string, status?: number) => void) { this.#onError = f; } @@ -326,8 +318,6 @@ class MeasurementEngine { } #next(): void { - if (!this.#running) return; - const resumeType = this.#curType(); const resumeResults = this.#curTypeResults(); if ( @@ -484,10 +474,6 @@ class MeasurementEngine { }; engine!.onConnectionError = (e: unknown) => { - if (isAuthorizationRejection(e)) { - this.#onError(e); - return; - } msmResults.error = e; this.onResultsChange({ type }); this.#onError(`Connection error while measuring packet loss: ${e}`); @@ -575,9 +561,10 @@ class MeasurementEngine { this.#running && this.#next(); }; - engine.onConnectionError = (e: unknown) => { - if (isAuthorizationRejection(e)) { - this.#onError(e); + engine.onConnectionError = (e: unknown, status?: number) => { + if (status !== undefined) { + this.#setRunning(false); + this.#onError(String(e), status); return; } this.#serverTimeDelta = (engine as BandwidthEngine).serverTimeDelta; @@ -734,9 +721,10 @@ class MeasurementEngine { this.#running && this.#next(); }; - engine.onConnectionError = (e: unknown) => { - if (isAuthorizationRejection(e)) { - this.#onError(e); + engine.onConnectionError = (e: unknown, status?: number) => { + if (status !== undefined) { + this.#setRunning(false); + this.#onError(String(e), status); return; } this.#serverTimeDelta = (engine as BandwidthEngine).serverTimeDelta; diff --git a/tests/e2e/httpError.test.ts b/tests/e2e/httpError.test.ts new file mode 100644 index 0000000..b49e455 --- /dev/null +++ b/tests/e2e/httpError.test.ts @@ -0,0 +1,88 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; +import SpeedTest from '../../src'; +import BandwidthEngine from '../../src/engines/BandwidthEngine/BandwidthEngine'; + +const originalFetch = window.fetch; + +afterEach(() => { + window.fetch = originalFetch; + vi.restoreAllMocks(); +}); + +describe.each([400, 401, 403, 500])('bandwidth HTTP %i', status => { + it('stops the top-level engine without retrying or advancing', async () => { + const fetchMock = vi.fn(() => + Promise.resolve(new Response(null, { status })) + ); + window.fetch = fetchMock; + vi.spyOn(console, 'warn').mockImplementation(() => undefined); + + const engine = new SpeedTest({ + autoStart: false, + downloadApiUrl: 'https://example.com/down', + uploadApiUrl: 'https://example.com/up', + logAimApiUrl: null, + logMeasurementApiUrl: null, + measurements: [ + { type: 'latency', numPackets: 1 }, + { type: 'download', bytes: 1_000, count: 1 } + ] + }); + const phases: string[] = []; + const onFinish = vi.fn(); + engine.onPhaseChange = ({ measurement }) => phases.push(measurement.type); + engine.onFinish = onFinish; + + const error = new Promise<{ + message: string; + running: boolean; + status?: number; + }>(resolve => { + engine.onError = (message, responseStatus) => { + resolve({ + message, + running: engine.isRunning, + status: responseStatus + }); + }; + }); + + engine.play(); + + await expect(error).resolves.toEqual({ + message: expect.stringMatching( + new RegExp(`^Request failed with ${status}: https://example\\.com/down`) + ), + running: false, + status + }); + await new Promise(resolve => setTimeout(resolve, 20)); + + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(phases).toEqual(['latency']); + expect(onFinish).not.toHaveBeenCalled(); + }); +}); + +it('retains the existing retry behavior for network failures', async () => { + const fetchMock = vi.fn(() => Promise.reject(new TypeError('fetch failed'))); + window.fetch = fetchMock; + vi.spyOn(console, 'warn').mockImplementation(() => undefined); + + const engine = new BandwidthEngine([{ dir: 'down', bytes: 0, count: 1 }], { + downloadApiUrl: 'https://example.com/down', + uploadApiUrl: 'https://example.com/up' + }); + const error = new Promise<{ message: string; status?: number }>(resolve => { + engine.onConnectionError = (message, status) => + resolve({ message, status }); + }); + + engine.play(); + + await expect(error).resolves.toEqual({ + message: expect.stringContaining('Gave up after 20 retries.'), + status: undefined + }); + expect(fetchMock).toHaveBeenCalledTimes(21); +}); diff --git a/tests/unit/config/authorizationToken.test.ts b/tests/unit/config/authorizationToken.test.ts index acf1541..3ea9174 100644 --- a/tests/unit/config/authorizationToken.test.ts +++ b/tests/unit/config/authorizationToken.test.ts @@ -11,10 +11,6 @@ class InspectableEngine extends SpeedTestEngine { get resolvedConfig() { return this.config; } - - get resolvedAuthorization() { - return this.authorization; - } } const API_URL_KEYS = [ @@ -55,17 +51,6 @@ describe('authorizationToken', () => { ).toBe(TOKEN); }); - it('replaces the token used by future requests', () => { - const engine = new InspectableEngine({ - autoStart: false, - authorizationToken: TOKEN - }); - - engine.setAuthorizationToken('replacement-token'); - - expect(engine.resolvedAuthorization.token).toBe('replacement-token'); - }); - it('defaults to null', () => { expect(resolveConfig({}).authorizationToken).toBeNull(); expect(defaultConfig.authorizationToken).toBeNull(); From 1aa77d7cb46d37c4bae5d516ea59e00747377334 Mon Sep 17 00:00:00 2001 From: apascoa Date: Wed, 2 Sep 2026 17:29:05 +0100 Subject: [PATCH 3/5] fix: preserve transient bandwidth retries --- .../BandwidthEngine/BandwidthEngine.ts | 10 ++- .../BandwidthEngine/ParallelLatency.ts | 6 ++ tests/e2e/httpError.test.ts | 67 ++++++++++++++++++- 3 files changed, 80 insertions(+), 3 deletions(-) diff --git a/src/engines/BandwidthEngine/BandwidthEngine.ts b/src/engines/BandwidthEngine/BandwidthEngine.ts index dcf814c..6480cd8 100644 --- a/src/engines/BandwidthEngine/BandwidthEngine.ts +++ b/src/engines/BandwidthEngine/BandwidthEngine.ts @@ -576,7 +576,12 @@ class BandwidthMeasurementEngine implements Engine { } console.warn(`Error fetching ${url}: ${error}`); - if (error instanceof HttpError) { + const retryable = + !(error instanceof HttpError) || + error.status === 408 || + error.status === 429 || + (error.status >= 500 && error.status <= 599); + if (!retryable) { this.#retries = 0; this.#setRunning(false); this.#onConnectionError( @@ -589,7 +594,8 @@ class BandwidthMeasurementEngine implements Engine { this.#retries = 0; this.#setRunning(false); this.#onConnectionError( - `Connection failed to ${url}. Gave up after ${MAX_RETRIES} retries.` + `Connection failed to ${url}. Gave up after ${MAX_RETRIES} retries.`, + error instanceof HttpError ? error.status : undefined ); } }); diff --git a/src/engines/BandwidthEngine/ParallelLatency.ts b/src/engines/BandwidthEngine/ParallelLatency.ts index 2911a77..b29fef6 100644 --- a/src/engines/BandwidthEngine/ParallelLatency.ts +++ b/src/engines/BandwidthEngine/ParallelLatency.ts @@ -103,6 +103,12 @@ class BandwidthWithParallelLatencyEngine extends BandwidthEngine { this.#latencyEngine && this.#latencyEngine.pause(); onConnectionError(...args); }; + if (this.#latencyEngine) { + this.#latencyEngine.onConnectionError = (...args: [string, number?]) => { + this.pause(); + onConnectionError(...args); + }; + } } // Internal state diff --git a/tests/e2e/httpError.test.ts b/tests/e2e/httpError.test.ts index b49e455..96ba11f 100644 --- a/tests/e2e/httpError.test.ts +++ b/tests/e2e/httpError.test.ts @@ -9,7 +9,7 @@ afterEach(() => { vi.restoreAllMocks(); }); -describe.each([400, 401, 403, 500])('bandwidth HTTP %i', status => { +describe.each([400, 401, 403])('bandwidth HTTP %i', status => { it('stops the top-level engine without retrying or advancing', async () => { const fetchMock = vi.fn(() => Promise.resolve(new Response(null, { status })) @@ -64,6 +64,71 @@ describe.each([400, 401, 403, 500])('bandwidth HTTP %i', status => { }); }); +describe.each([408, 429, 500])('transient bandwidth HTTP %i', status => { + it('uses the existing retry budget', async () => { + const fetchMock = vi.fn(() => + Promise.resolve(new Response(null, { status })) + ); + window.fetch = fetchMock; + vi.spyOn(console, 'warn').mockImplementation(() => undefined); + + const engine = new BandwidthEngine([{ dir: 'down', bytes: 0, count: 1 }], { + downloadApiUrl: 'https://example.com/down', + uploadApiUrl: 'https://example.com/up' + }); + const error = new Promise<{ message: string; status?: number }>(resolve => { + engine.onConnectionError = (message, responseStatus) => + resolve({ message, status: responseStatus }); + }); + + engine.play(); + + await expect(error).resolves.toEqual({ + message: expect.stringContaining('Gave up after 20 retries.'), + status + }); + expect(fetchMock).toHaveBeenCalledTimes(21); + }); +}); + +it('forwards terminal errors from the loaded-latency engine', async () => { + const fetchMock = vi.fn( + (input: RequestInfo | URL, init?: RequestInit): Promise => { + const url = new URL(input.toString()); + if (url.searchParams.get('bytes') === '0') { + return Promise.resolve(new Response(null, { status: 401 })); + } + return new Promise((_resolve, reject) => { + init?.signal?.addEventListener('abort', () => + reject(init.signal?.reason) + ); + }); + } + ); + window.fetch = fetchMock; + vi.spyOn(console, 'warn').mockImplementation(() => undefined); + + const engine = new SpeedTest({ + autoStart: false, + downloadApiUrl: 'https://example.com/down', + uploadApiUrl: 'https://example.com/up', + logAimApiUrl: null, + logMeasurementApiUrl: null, + measureDownloadLoadedLatency: true, + measurements: [{ type: 'download', bytes: 1_000, count: 1 }] + }); + const error = new Promise<{ running: boolean; status?: number }>(resolve => { + engine.onError = (_message, status) => { + resolve({ running: engine.isRunning, status }); + }; + }); + + engine.play(); + + await expect(error).resolves.toEqual({ running: false, status: 401 }); + expect(fetchMock).toHaveBeenCalledTimes(2); +}); + it('retains the existing retry behavior for network failures', async () => { const fetchMock = vi.fn(() => Promise.reject(new TypeError('fetch failed'))); window.fetch = fetchMock; From 90eebf2f30695e1fe9a5f131f6652d42a6b6ee4a Mon Sep 17 00:00:00 2001 From: apascoa Date: Thu, 3 Sep 2026 15:39:04 +0100 Subject: [PATCH 4/5] fix: stop tests after bandwidth errors --- .../BandwidthEngine/BandwidthEngine.ts | 69 ++++++++++++------- src/index.ts | 30 +++----- tests/e2e/httpError.test.ts | 49 +++++++++---- 3 files changed, 89 insertions(+), 59 deletions(-) diff --git a/src/engines/BandwidthEngine/BandwidthEngine.ts b/src/engines/BandwidthEngine/BandwidthEngine.ts index 6480cd8..598bd72 100644 --- a/src/engines/BandwidthEngine/BandwidthEngine.ts +++ b/src/engines/BandwidthEngine/BandwidthEngine.ts @@ -4,17 +4,28 @@ import { type AuthorizationOptions } from '../../utils/authorization'; -const MAX_RETRIES = 20; +const MAX_RETRIES = 3; class HttpError extends Error { constructor( readonly status: number, - statusText: string + statusText: string, + readonly retryAfter: string | null ) { super(statusText); } } +const getRetryDelay = (retryAfter: string | null): number => { + if (!retryAfter) return 5000; + + const seconds = Number(retryAfter); + if (Number.isFinite(seconds) && seconds >= 0) return seconds * 1000; + + const date = Date.parse(retryAfter); + return Number.isNaN(date) ? 5000 : Math.max(0, date - Date.now()); +}; + const ESTIMATED_HEADER_FRACTION = 0.005; // ~.5% of packet header / payload size. used when transferSize is not available. const SERVER_TIME_MIN_DURATION = 0.01; // minimum server-provided duration value to consider valid (ms) @@ -251,7 +262,7 @@ class BandwidthMeasurementEngine implements Engine { } play(): void { - if (!this.#running) { + if (!this.#failed && !this.#running) { this.#setRunning(true); this.#nextMeasurement(); } @@ -263,6 +274,7 @@ class BandwidthMeasurementEngine implements Engine { #uploadApi: string; #running: boolean = false; + #failed: boolean = false; #finished: Record = { down: false, up: false }; #results: BandwidthEngineResults = { down: {}, up: {} }; #measIdx: number = 0; @@ -415,9 +427,9 @@ class BandwidthMeasurementEngine implements Engine { if (this.abortRequestDuration) { const abortTimeout = setTimeout(() => { const errorMessage = `${isDown ? 'Download' : 'Upload'} measurement of ${numBytes} bytes aborted. Measurement exceeded bandwidthAbortRequestDuration (${this.abortRequestDuration}ms)`; - this.#cancelCurrentMeasurement(errorMessage); this.#retries = 0; - this.#setRunning(false); + this.pause(); + this.#failed = true; this.#onConnectionError(errorMessage); }, this.abortRequestDuration); this.#currentAbortController.signal.addEventListener('abort', () => @@ -433,7 +445,11 @@ class BandwidthMeasurementEngine implements Engine { }) .then(r => { if (r.ok) return r; - throw new HttpError(r.status, r.statusText); + throw new HttpError( + r.status, + r.statusText, + r.headers.get('retry-after') + ); }) .then(r => { this.getServerTime && (serverTime = this.getServerTime(r)); @@ -576,28 +592,29 @@ class BandwidthMeasurementEngine implements Engine { } console.warn(`Error fetching ${url}: ${error}`); - const retryable = - !(error instanceof HttpError) || - error.status === 408 || - error.status === 429 || - (error.status >= 500 && error.status <= 599); - if (!retryable) { - this.#retries = 0; - this.#setRunning(false); - this.#onConnectionError( - `Request failed with ${error.status}: ${url}`, - error.status - ); - } else if (this.#retries++ < MAX_RETRIES) { - this.#nextMeasurement(); // keep trying - } else { - this.#retries = 0; - this.#setRunning(false); - this.#onConnectionError( - `Connection failed to ${url}. Gave up after ${MAX_RETRIES} retries.`, - error instanceof HttpError ? error.status : undefined + if ( + error instanceof HttpError && + error.status === 429 && + this.#retries++ < MAX_RETRIES + ) { + setTimeout( + () => this.#nextMeasurement(), + getRetryDelay(error.retryAfter) ); + return; } + + const status = error instanceof HttpError ? error.status : undefined; + const message = + status === 429 + ? `Connection failed to ${url}. Gave up after ${MAX_RETRIES} retries.` + : status === undefined + ? `Connection failed to ${url}.` + : `Request failed with ${status}: ${url}`; + this.#retries = 0; + this.pause(); + this.#failed = true; + this.#onConnectionError(message, status); }); } diff --git a/src/index.ts b/src/index.ts index b85262e..ba276c5 100644 --- a/src/index.ts +++ b/src/index.ts @@ -201,7 +201,7 @@ class MeasurementEngine { * resumes the current phase. */ play(): void { - if (!this.#running) { + if (!this.#failed && !this.#running) { // Clear timings before running the engine performance.clearResourceTimings(); @@ -244,6 +244,7 @@ class MeasurementEngine { #running: boolean = false; #finished: boolean = false; + #failed: boolean = false; // Internal methods #setRunning(running: boolean): void { @@ -297,6 +298,7 @@ class MeasurementEngine { this.#setRunning(false); this.#setFinished(false); + this.#failed = false; this.#results.clear(); this.#accumulatedRuntimeMs = 0; @@ -562,16 +564,9 @@ class MeasurementEngine { }; engine.onConnectionError = (e: unknown, status?: number) => { - if (status !== undefined) { - this.#setRunning(false); - this.#onError(String(e), status); - return; - } - this.#serverTimeDelta = (engine as BandwidthEngine).serverTimeDelta; - msmResults.error = e; - this.onResultsChange({ type }); - this.#onError(`Connection error while measuring latency: ${e}`); - this.#next(); + this.#failed = true; + this.pause(); + this.#onError(String(e), status); }; (engine as Engine & { play: () => void }).play!(); @@ -722,16 +717,9 @@ class MeasurementEngine { }; engine.onConnectionError = (e: unknown, status?: number) => { - if (status !== undefined) { - this.#setRunning(false); - this.#onError(String(e), status); - return; - } - this.#serverTimeDelta = (engine as BandwidthEngine).serverTimeDelta; - msmResults.error = e; - this.onResultsChange({ type }); - this.#onError(`Connection error while measuring ${type}: ${e}`); - this.#next(); + this.#failed = true; + this.pause(); + this.#onError(String(e), status); }; (engine as Engine & { play: () => void }).play!(); diff --git a/tests/e2e/httpError.test.ts b/tests/e2e/httpError.test.ts index 96ba11f..f18a724 100644 --- a/tests/e2e/httpError.test.ts +++ b/tests/e2e/httpError.test.ts @@ -6,11 +6,12 @@ const originalFetch = window.fetch; afterEach(() => { window.fetch = originalFetch; + vi.useRealTimers(); vi.restoreAllMocks(); }); -describe.each([400, 401, 403])('bandwidth HTTP %i', status => { - it('stops the top-level engine without retrying or advancing', async () => { +describe.each([400, 401, 403, 408, 500])('bandwidth HTTP %i', status => { + it('stops the top-level engine without retrying, advancing, or resuming', async () => { const fetchMock = vi.fn(() => Promise.resolve(new Response(null, { status })) ); @@ -61,13 +62,26 @@ describe.each([400, 401, 403])('bandwidth HTTP %i', status => { expect(fetchMock).toHaveBeenCalledTimes(1); expect(phases).toEqual(['latency']); expect(onFinish).not.toHaveBeenCalled(); + + engine.play(); + await new Promise(resolve => setTimeout(resolve, 20)); + expect(fetchMock).toHaveBeenCalledTimes(1); }); }); -describe.each([408, 429, 500])('transient bandwidth HTTP %i', status => { - it('uses the existing retry budget', async () => { +describe.each([ + ['uses Retry-After', '2', 2_000], + ['waits five seconds when Retry-After is missing', null, 5_000] +])('HTTP 429 %s', (_label, retryAfter, delay) => { + it('retries three times before failing', async () => { + vi.useFakeTimers(); const fetchMock = vi.fn(() => - Promise.resolve(new Response(null, { status })) + Promise.resolve( + new Response(null, { + status: 429, + headers: retryAfter ? { 'retry-after': retryAfter } : undefined + }) + ) ); window.fetch = fetchMock; vi.spyOn(console, 'warn').mockImplementation(() => undefined); @@ -83,15 +97,22 @@ describe.each([408, 429, 500])('transient bandwidth HTTP %i', status => { engine.play(); + await vi.advanceTimersByTimeAsync(0); + for (let retry = 1; retry <= 3; retry++) { + await vi.advanceTimersByTimeAsync(delay - 1); + expect(fetchMock).toHaveBeenCalledTimes(retry); + await vi.advanceTimersByTimeAsync(1); + expect(fetchMock).toHaveBeenCalledTimes(retry + 1); + } + await expect(error).resolves.toEqual({ - message: expect.stringContaining('Gave up after 20 retries.'), - status + message: expect.stringContaining('Gave up after 3 retries.'), + status: 429 }); - expect(fetchMock).toHaveBeenCalledTimes(21); }); }); -it('forwards terminal errors from the loaded-latency engine', async () => { +it('stops and cannot resume after a loaded-latency error', async () => { const fetchMock = vi.fn( (input: RequestInfo | URL, init?: RequestInit): Promise => { const url = new URL(input.toString()); @@ -127,9 +148,13 @@ it('forwards terminal errors from the loaded-latency engine', async () => { await expect(error).resolves.toEqual({ running: false, status: 401 }); expect(fetchMock).toHaveBeenCalledTimes(2); + + engine.play(); + await new Promise(resolve => setTimeout(resolve, 20)); + expect(fetchMock).toHaveBeenCalledTimes(2); }); -it('retains the existing retry behavior for network failures', async () => { +it('stops immediately after a network failure', async () => { const fetchMock = vi.fn(() => Promise.reject(new TypeError('fetch failed'))); window.fetch = fetchMock; vi.spyOn(console, 'warn').mockImplementation(() => undefined); @@ -146,8 +171,8 @@ it('retains the existing retry behavior for network failures', async () => { engine.play(); await expect(error).resolves.toEqual({ - message: expect.stringContaining('Gave up after 20 retries.'), + message: expect.stringContaining('Connection failed'), status: undefined }); - expect(fetchMock).toHaveBeenCalledTimes(21); + expect(fetchMock).toHaveBeenCalledTimes(1); }); From bda6327790ab418b6c93c92958027f0fcdbae22f Mon Sep 17 00:00:00 2001 From: apascoa Date: Tue, 8 Sep 2026 11:58:42 +0100 Subject: [PATCH 5/5] fix: finalize bandwidth error handling --- .../BandwidthEngine/BandwidthEngine.ts | 19 +++-- src/index.ts | 4 + tests/e2e/httpError.test.ts | 75 +++++++++++++++++++ 3 files changed, 93 insertions(+), 5 deletions(-) diff --git a/src/engines/BandwidthEngine/BandwidthEngine.ts b/src/engines/BandwidthEngine/BandwidthEngine.ts index 598bd72..40cf34a 100644 --- a/src/engines/BandwidthEngine/BandwidthEngine.ts +++ b/src/engines/BandwidthEngine/BandwidthEngine.ts @@ -257,6 +257,10 @@ class BandwidthMeasurementEngine implements Engine { // Public methods pause(): void { + if (this.#retryTimeout !== undefined) { + clearTimeout(this.#retryTimeout); + this.#retryTimeout = undefined; + } this.#cancelCurrentMeasurement(`pause()`); this.#setRunning(false); } @@ -280,6 +284,7 @@ class BandwidthMeasurementEngine implements Engine { #measIdx: number = 0; #counter: number = 0; #retries: number = 0; + #retryTimeout: ReturnType | undefined; #minDuration: number = -Infinity; // of current measurement #throttleMs: number = 0; #estimatedServerTime: number = 0; @@ -418,7 +423,11 @@ class BandwidthMeasurementEngine implements Engine { url ); - if (this.#retries === 0) { + if ( + this.#retries === 0 || + !this.#currentAbortController || + this.#currentAbortController.signal.aborted + ) { // abort existing abort controller this.#currentAbortController?.abort('restarting engine'); @@ -597,10 +606,10 @@ class BandwidthMeasurementEngine implements Engine { error.status === 429 && this.#retries++ < MAX_RETRIES ) { - setTimeout( - () => this.#nextMeasurement(), - getRetryDelay(error.retryAfter) - ); + this.#retryTimeout = setTimeout(() => { + this.#retryTimeout = undefined; + if (this.#running && !this.#failed) this.#nextMeasurement(); + }, getRetryDelay(error.retryAfter)); return; } diff --git a/src/index.ts b/src/index.ts index ba276c5..b419735 100644 --- a/src/index.ts +++ b/src/index.ts @@ -564,8 +564,10 @@ class MeasurementEngine { }; engine.onConnectionError = (e: unknown, status?: number) => { + msmResults.error = e; this.#failed = true; this.pause(); + this.onResultsChange({ type }); this.#onError(String(e), status); }; @@ -717,8 +719,10 @@ class MeasurementEngine { }; engine.onConnectionError = (e: unknown, status?: number) => { + msmResults.error = e; this.#failed = true; this.pause(); + this.onResultsChange({ type }); this.#onError(String(e), status); }; diff --git a/tests/e2e/httpError.test.ts b/tests/e2e/httpError.test.ts index f18a724..0d069ab 100644 --- a/tests/e2e/httpError.test.ts +++ b/tests/e2e/httpError.test.ts @@ -30,8 +30,15 @@ describe.each([400, 401, 403, 408, 500])('bandwidth HTTP %i', status => { ] }); const phases: string[] = []; + const resultErrors: string[] = []; const onFinish = vi.fn(); engine.onPhaseChange = ({ measurement }) => phases.push(measurement.type); + engine.onResultsChange = ({ type }) => { + const results = engine.results.raw[type]; + if (typeof results === 'object' && results.error) { + resultErrors.push(results.error); + } + }; engine.onFinish = onFinish; const error = new Promise<{ @@ -61,6 +68,9 @@ describe.each([400, 401, 403, 408, 500])('bandwidth HTTP %i', status => { expect(fetchMock).toHaveBeenCalledTimes(1); expect(phases).toEqual(['latency']); + expect(resultErrors).toEqual([ + expect.stringMatching(`^Request failed with ${status}:`) + ]); expect(onFinish).not.toHaveBeenCalled(); engine.play(); @@ -112,6 +122,63 @@ describe.each([ }); }); +it('cancels a pending 429 retry when paused', async () => { + vi.useFakeTimers(); + const fetchMock = vi.fn(() => + Promise.resolve( + new Response(null, { + status: 429, + headers: { 'retry-after': '1' } + }) + ) + ); + window.fetch = fetchMock; + vi.spyOn(console, 'warn').mockImplementation(() => undefined); + + const engine = new BandwidthEngine([{ dir: 'down', bytes: 0, count: 1 }], { + downloadApiUrl: 'https://example.com/down', + uploadApiUrl: 'https://example.com/up' + }); + + engine.play(); + await vi.advanceTimersByTimeAsync(0); + engine.pause(); + await vi.advanceTimersByTimeAsync(1_000); + + expect(fetchMock).toHaveBeenCalledTimes(1); +}); + +it('cancels the old engine 429 retry when restarted', async () => { + vi.useFakeTimers(); + const fetchMock = vi + .fn() + .mockResolvedValueOnce( + new Response(null, { + status: 429, + headers: { 'retry-after': '1' } + }) + ) + .mockResolvedValue(new Response(null, { status: 401 })); + window.fetch = fetchMock; + vi.spyOn(console, 'warn').mockImplementation(() => undefined); + + const engine = new SpeedTest({ + autoStart: false, + downloadApiUrl: 'https://example.com/down', + uploadApiUrl: 'https://example.com/up', + logAimApiUrl: null, + logMeasurementApiUrl: null, + measurements: [{ type: 'download', bytes: 1_000, count: 1 }] + }); + + engine.play(); + await vi.advanceTimersByTimeAsync(0); + engine.restart(); + await vi.advanceTimersByTimeAsync(1_000); + + expect(fetchMock).toHaveBeenCalledTimes(2); +}); + it('stops and cannot resume after a loaded-latency error', async () => { const fetchMock = vi.fn( (input: RequestInfo | URL, init?: RequestInit): Promise => { @@ -138,6 +205,13 @@ it('stops and cannot resume after a loaded-latency error', async () => { measureDownloadLoadedLatency: true, measurements: [{ type: 'download', bytes: 1_000, count: 1 }] }); + const resultErrors: string[] = []; + engine.onResultsChange = ({ type }) => { + const results = engine.results.raw[type]; + if (typeof results === 'object' && results.error) { + resultErrors.push(results.error); + } + }; const error = new Promise<{ running: boolean; status?: number }>(resolve => { engine.onError = (_message, status) => { resolve({ running: engine.isRunning, status }); @@ -148,6 +222,7 @@ it('stops and cannot resume after a loaded-latency error', async () => { await expect(error).resolves.toEqual({ running: false, status: 401 }); expect(fetchMock).toHaveBeenCalledTimes(2); + expect(resultErrors).toEqual([expect.stringContaining('Request failed')]); engine.play(); await new Promise(resolve => setTimeout(resolve, 20));