From d6aa7ec6739c55531cafed512407b6540f73d92a Mon Sep 17 00:00:00 2001 From: gomes <17035424+gomesalexandre@users.noreply.github.com> Date: Tue, 1 Sep 2026 15:19:02 +0200 Subject: [PATCH 1/2] fix: fetchWithErrorHandling timeout is a complete no-op The await inside the Promise.race array resolved the fetch to completion before the race was even constructed, so the timeout promise raced against an already-settled value and could never win. Move the await outside the race, matching the pattern already used by timeoutFetch and safelyExecuteWithTimeout in this same file. Also correct the timeout promise's type from Promise (it never resolves to a Response - it only ever rejects) to Promise, matching safelyExecuteWithTimeout's existing pattern. fetchWithErrorHandling had zero unit tests before this change - add two, one covering the existing fast-path behavior and one proving the timeout is now honored (fails on the prior implementation). --- packages/controller-utils/CHANGELOG.md | 5 +++ packages/controller-utils/src/util.test.ts | 41 ++++++++++++++++++++++ packages/controller-utils/src/util.ts | 6 ++-- 3 files changed, 49 insertions(+), 3 deletions(-) diff --git a/packages/controller-utils/CHANGELOG.md b/packages/controller-utils/CHANGELOG.md index 487e484d820..1f246dd2faf 100644 --- a/packages/controller-utils/CHANGELOG.md +++ b/packages/controller-utils/CHANGELOG.md @@ -36,6 +36,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `handleWhen` - These symbols will be removed in a future major version. Please use equivalent implementations from `@metamask/base-data-service` going forward. +### Fixed + +- Fix `fetchWithErrorHandling` ignoring its `timeout` option ([#PRNUM](https://github.com/MetaMask/core/pull/PRNUM)) + - Previously, the fetch was awaited before `Promise.race` was constructed, so the race always resolved to the already-settled fetch and the timeout could never fire. + ## [12.3.0] ### Added diff --git a/packages/controller-utils/src/util.test.ts b/packages/controller-utils/src/util.test.ts index e3346ea473f..d029c63e350 100644 --- a/packages/controller-utils/src/util.test.ts +++ b/packages/controller-utils/src/util.test.ts @@ -561,6 +561,47 @@ describe('util', () => { }); }); + describe('fetchWithErrorHandling', () => { + it('should fetch first if response is faster than timeout', async () => { + nock(SOME_API).get(/.+/u).delay(50).reply(200, { foo: 'bar' }); + const result = await util.fetchWithErrorHandling({ + url: SOME_API, + timeout: 300, + }); + expect(result).toStrictEqual({ foo: 'bar' }); + }); + + it('should stop waiting once the timeout elapses, even if the fetch never resolves in time', async () => { + nock(SOME_API).get(/.+/u).delay(300).reply(200, { foo: 'bar' }); + const consoleErrorSpy = jest + .spyOn(console, 'error') + .mockImplementation(); + const start = Date.now(); + const result = await util.fetchWithErrorHandling({ + url: SOME_API, + timeout: 50, + }); + const elapsed = Date.now() - start; + // The timeout error is logged (not thrown) and the result is + // undefined, matching how a caught+swallowed error already behaves. + expect(result).toBeUndefined(); + expect(consoleErrorSpy).toHaveBeenCalledWith( + expect.objectContaining({ message: 'timeout' }), + ); + // If the timeout is honored, this resolves in ~50ms, not ~300ms. + // On unfixed code, the outer `await` inside the race already + // resolves the fetch before the race is constructed, so this takes + // the full 300ms instead. The bound is loose (well under the 300ms + // delay, generous above the 50ms timeout) to avoid CI flakiness on a + // loaded worker; the `result` assertion above is the primary proof. + expect(elapsed).toBeLessThan(250); + consoleErrorSpy.mockRestore(); + // Let the abandoned nock interceptor settle before the test ends, + // so it doesn't leave a dangling timer past the process's teardown. + await new Promise((resolve) => setTimeout(resolve, 300)); + }); + }); + describe('normalizeEnsName', () => { it('should normalize with valid 2LD', async () => { let valid = util.normalizeEnsName('metamask.eth'); diff --git a/packages/controller-utils/src/util.ts b/packages/controller-utils/src/util.ts index 71a0f8c5430..6472fb7123a 100644 --- a/packages/controller-utils/src/util.ts +++ b/packages/controller-utils/src/util.ts @@ -486,9 +486,9 @@ export async function fetchWithErrorHandling({ let result; try { if (timeout) { - result = Promise.race([ - await handleFetch(url, options), - new Promise((_resolve, reject) => + result = await Promise.race([ + handleFetch(url, options), + new Promise((_resolve, reject) => setTimeout(() => { reject(TIMEOUT_ERROR); }, timeout), From 103c3ff6e5031e58920bcc4b7d487b2520044f95 Mon Sep 17 00:00:00 2001 From: gomes <17035424+gomesalexandre@users.noreply.github.com> Date: Tue, 1 Sep 2026 15:19:59 +0200 Subject: [PATCH 2/2] chore: backfill real PR number in changelog entry --- packages/controller-utils/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/controller-utils/CHANGELOG.md b/packages/controller-utils/CHANGELOG.md index 1f246dd2faf..92c606edc1b 100644 --- a/packages/controller-utils/CHANGELOG.md +++ b/packages/controller-utils/CHANGELOG.md @@ -38,7 +38,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- Fix `fetchWithErrorHandling` ignoring its `timeout` option ([#PRNUM](https://github.com/MetaMask/core/pull/PRNUM)) +- Fix `fetchWithErrorHandling` ignoring its `timeout` option ([#10048](https://github.com/MetaMask/core/pull/10048)) - Previously, the fetch was awaited before `Promise.race` was constructed, so the race always resolved to the already-settled fetch and the timeout could never fire. ## [12.3.0]