Skip to content

fix: fetchWithErrorHandling timeout is a complete no-op - #10048

Open
gomesalexandre wants to merge 2 commits into
MetaMask:mainfrom
gomesalexandre:fix_fetch_with_error_handling_timeout_noop
Open

fix: fetchWithErrorHandling timeout is a complete no-op#10048
gomesalexandre wants to merge 2 commits into
MetaMask:mainfrom
gomesalexandre:fix_fetch_with_error_handling_timeout_noop

Conversation

@gomesalexandre

@gomesalexandre gomesalexandre commented Sep 1, 2026

Copy link
Copy Markdown

Summary

fetchWithErrorHandling's timeout option is a complete no-op — it has been since it was introduced. The fetch is fully awaited before Promise.race is even constructed, so the "race" runs an already-settled promise against a fresh timer, and the settled value always wins.

// packages/controller-utils/src/util.ts:489-497 (before)
result = Promise.race([
  await handleFetch(url, options),   // <- awaited HERE, unbounded
  new Promise<Response>((_resolve, reject) =>
    setTimeout(() => reject(TIMEOUT_ERROR), timeout),
  ),
]);

timeoutFetch (15 lines below, same file) and safelyExecuteWithTimeout (~line 267, same file) both do this correctly — the await sits outside the array. fetchWithErrorHandling is the only place in this ~40-package monorepo where an await appears inside a Promise.race([...]) array (checked via grep -rn "Promise.race(\[" packages/ across every occurrence).

This had zero unit tests before this PR (grep -rn "fetchWithErrorHandling" packages/controller-utils/ only turned up the export, the definition, and a name-listing assertion in index.test.ts), which is exactly why it went unnoticed.

Live impact

Two callers pass a timeout and rely on it for graceful degradation against the third-party chainid.network:

  • packages/network-enablement-controller/src/services/Slip44Service.ts:96 (timeout: 10000) - memoized in #fetchPromise, so every concurrent caller blocks on the same hang.
  • packages/assets-controller/src/utils/native-assets.ts:49.

React Native's fetch has no default timeout, so on RN this hangs to the OS TCP timeout (can be minutes) instead of the intended 10s. This is an availability/hang bug, not a fund-safety issue - both callers already handle an undefined result by falling back to a static/seed value, so with the fix that fallback now actually triggers within the intended window instead of after an unbounded hang.

Fix

Move the await outside the race, and correct the timeout promise's type from Promise<Response> (it never resolves to a Response - handleFetch returns parsed JSON, and the timeout promise only ever rejects) to Promise<never>, matching the existing safelyExecuteWithTimeout pattern in this same file.

receipts

New test added first against unfixed code to prove the bug (genuine red, not a hang - explicit test timeout confirmed the assertion, not a Jest-level timeout):

FAIL controller-utils src/util.test.ts
  ● util › fetchWithErrorHandling › should stop waiting once the timeout elapses, even if the fetch never resolves in time
    expect(received).toBeUndefined()
    Received: {"foo": "bar"}

(unfixed code waits the full mocked fetch delay and returns the fetched body - the timeout never fires)

Same test against the fix, plus the full package suite:

$ yarn jest
Test Suites: 6 passed, 6 total
Tests:       188 passed, 188 total
Snapshots:   1 passed, 1 total
$ yarn eslint packages/controller-utils/src/util.ts packages/controller-utils/src/util.test.ts
(clean, no output)

$ yarn tsc --build tsconfig.build.json
(clean, exit 0)

$ yarn changelog:validate
(clean, no output)

Adversarially reviewed with Codex (GPT-5.6) before opening. It confirmed the fix is semantically correct and the test is not tautological (genuinely fails on old code, passes on new), flagged the elapsed-time assertion as a CI-flakiness risk on a loaded worker (loosened the bound, kept a comment explaining the primary proof is the returned value not the timing) and flagged a missing changelog entry (added, following this package's ### Fixed convention).

risk

Low - the non-timeout path (else { result = await handleFetch(url, options); }) is untouched. The only observable behavior change is that a timeout option now actually does what its name says.


Note

Low Risk
Behavior change is limited to callers that pass timeout; they now get timely fallback instead of unbounded hangs, with no change to the non-timeout path.

Overview
fetchWithErrorHandling in @metamask/controller-utils now honors its timeout option. The fetch was previously **await**ed inside the Promise.race array, so the race always saw an already-settled fetch and the timer could never win.

The fix races handleFetch against a rejecting timer promise (with await outside the race), and types the timer branch as Promise<never> to match other helpers in the same file. Calls without a timeout are unchanged.

New unit tests cover a response that finishes before the deadline and a slow response that stops waiting at the timeout (undefined result, timeout logged via existing error handling). The package changelog records the fix under Fixed.

Reviewed by Cursor Bugbot for commit 103c3ff. Bugbot is set up for automated code reviews on this repo. Configure here.

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<Response> (it never
resolves to a Response - it only ever rejects) to Promise<never>,
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).
@gomesalexandre
gomesalexandre marked this pull request as ready for review September 1, 2026 13:20
@gomesalexandre
gomesalexandre requested a review from a team as a code owner September 1, 2026 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant