fix: fetchWithErrorHandling timeout is a complete no-op - #10048
Open
gomesalexandre wants to merge 2 commits into
Open
fix: fetchWithErrorHandling timeout is a complete no-op#10048gomesalexandre wants to merge 2 commits into
gomesalexandre wants to merge 2 commits into
Conversation
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
marked this pull request as ready for review
September 1, 2026 13:20
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
fetchWithErrorHandling'stimeoutoption is a complete no-op — it has been since it was introduced. The fetch is fully awaited beforePromise.raceis even constructed, so the "race" runs an already-settled promise against a fresh timer, and the settled value always wins.timeoutFetch(15 lines below, same file) andsafelyExecuteWithTimeout(~line 267, same file) both do this correctly — theawaitsits outside the array.fetchWithErrorHandlingis the only place in this ~40-package monorepo where anawaitappears inside aPromise.race([...])array (checked viagrep -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 inindex.test.ts), which is exactly why it went unnoticed.Live impact
Two callers pass a
timeoutand rely on it for graceful degradation against the third-partychainid.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
fetchhas 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 anundefinedresult 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
awaitoutside the race, and correct the timeout promise's type fromPromise<Response>(it never resolves to aResponse-handleFetchreturns parsed JSON, and the timeout promise only ever rejects) toPromise<never>, matching the existingsafelyExecuteWithTimeoutpattern 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):
(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:
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
### Fixedconvention).risk
Low - the non-timeout path (
else { result = await handleFetch(url, options); }) is untouched. The only observable behavior change is that atimeoutoption 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
fetchWithErrorHandlingin@metamask/controller-utilsnow honors itstimeoutoption. The fetch was previously **await**ed inside thePromise.racearray, so the race always saw an already-settled fetch and the timer could never win.The fix races
handleFetchagainst a rejecting timer promise (withawaitoutside the race), and types the timer branch asPromise<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.