fix: fetchWithErrorHandling timeout is a complete no-op

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 ([#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]
 
 ### Added

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');

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<Response>((_resolve, reject) =>
+      result = await Promise.race([
+        handleFetch(url, options),
+        new Promise<never>((_resolve, reject) =>
           setTimeout(() => {
             reject(TIMEOUT_ERROR);
           }, timeout),